Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix[BMQ,MQB]: ensure Solaris build #561

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

678098
Copy link
Collaborator

@678098 678098 commented Jan 9, 2025

  1. Fix return value optimization problem with Solaris in bmqp::BlobPoolUtil::createBlobPool, by returning a shared pointer to the blob pool. The choice of shared pointer type is made to do it compatible with resource manager PR: Perf[BMQ,MQB]: resource manager for per-thread resources #499
  2. Update all places where blob shared pointer pool is used.
  3. bmqstoragetool: remove incompatible initializer list usage.
  4. bmqio_ntcchannel.t: disambiguate argument for ntcf::System::createInterface call.
  5. Logger classes: explicitly implement publish override to get rid of a warning.
  6. bmqu::BlobIterator remove unused const char* operator->() const; that produces a compilation warning Warning: Questionable (non-class) return type for BloombergLP::bmqu::BlobIterator::operator->() const.
  7. Replace unrecognized nullptr usage by NULL or bsl::nullptr_t() where applicable.
  8. mqbstat: pass enum template arguments in a way acceptable by Solaris compiler (BrokerStats::EventType::Enum::e_CLIENT_DESTROYED -> BrokerStats::EventType::e_CLIENT_DESTROYED).
  9. storage tool: set allocator flag via bmqtst::TestHelperUtil::ignoreCheckDefAlloc()

@678098 678098 requested a review from a team as a code owner January 9, 2025 21:19
@678098 678098 changed the title Fix[BMQ,MQB]: ensure Solaris build [WIP]Fix[BMQ,MQB]: ensure Solaris build Jan 9, 2025
@678098 678098 force-pushed the 250109_fix_BlobSpPool_creation branch 9 times, most recently from 2b8c80a to efbd130 Compare January 10, 2025 16:27
@678098 678098 changed the title [WIP]Fix[BMQ,MQB]: ensure Solaris build Fix[BMQ,MQB]: ensure Solaris build Jan 10, 2025
@678098 678098 requested a review from kaikulimu January 10, 2025 16:38
@kaikulimu
Copy link
Collaborator

  1. How did you test that the code is now compatible with Solaris compiler?
  2. There are a couple integration test failures. Could they be related to your changes?

@678098
Copy link
Collaborator Author

678098 commented Jan 10, 2025

@kaikulimu

  • There are a couple integration test failures. Could they be related to your changes?

These failures are also present in the build from the latest main:

https://github.com/bloomberg/blazingmq/actions/runs/12712681267/job/35439252483

kaikulimu
kaikulimu previously approved these changes Jan 10, 2025
Comment on lines 54 to 62
return bsl::shared_ptr<BlobSpPool>(
new (*alloc) BlobSpPool(
bdlf::BindUtil::bind(&createBlob,
blobBufferFactory_p,
bdlf::PlaceHolders::_1, // arena
bdlf::PlaceHolders::_2), // allocator
k_BLOB_POOL_GROWTH_STRATEGY,
alloc),
alloc);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return bsl::shared_ptr<BlobSpPool>(
new (*alloc) BlobSpPool(
bdlf::BindUtil::bind(&createBlob,
blobBufferFactory_p,
bdlf::PlaceHolders::_1, // arena
bdlf::PlaceHolders::_2), // allocator
k_BLOB_POOL_GROWTH_STRATEGY,
alloc),
alloc);
return bsl::allocate_shared<BlobSpPool>(
alloc,
bdlf::BindUtil::bind(&createBlob,
blobBufferFactory_p,
bdlf::PlaceHolders::_1, // arena
bdlf::PlaceHolders::_2), // allocator
k_BLOB_POOL_GROWTH_STRATEGY));

Slightly reduces the number of allocator shenanigans. allocate_shared should forward the allocator to the right BlobSpPool constructor and provide it to the resulting shared_ptr.

@@ -925,7 +925,7 @@ void Domain::removeDomainReset()
bslmt::LockGuard<bslmt::Mutex> guard(&d_mutex); // LOCK

d_state = e_PREREMOVE;
d_teardownRemoveCb = nullptr;
d_teardownRemoveCb = bsl::nullptr_t();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the use of nullptr_t's constructor a little controversial, I feel as if we should really only be using preexisting representations of a nullptr (e.g. NULL, nullptr) but I suppose this works?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the use of nullptr_t's constructor a little controversial, I feel as if we should really only be using preexisting representations of a nullptr (e.g. NULL, nullptr) but I suppose this works?

Pure nullptr doesn't work on Solaris, NULL or bsl::nullptr_t() work, but bsl::nullptr_t() is more restrictive. We typically used bsl::nullptr_t() to reset callbacks (bsl::function)

@678098 678098 merged commit e19ff33 into bloomberg:main Jan 10, 2025
28 of 30 checks passed
@678098 678098 deleted the 250109_fix_BlobSpPool_creation branch January 10, 2025 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants