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

Refactor[BMQT]: make QueueOptions rule-of-three complaint #371

Merged
merged 3 commits into from
Aug 13, 2024

Conversation

678098
Copy link
Collaborator

@678098 678098 commented Jul 23, 2024

Remove 400 lines from build logs

@678098 678098 force-pushed the 240723_bmqt_QueueOptions_rule_of_3 branch from 71ee553 to 6c94a4d Compare July 23, 2024 16:43
}
else {
d_subscriptions = Subscriptions(rhs.d_subscriptions,
rhs.d_allocator_p);
Copy link
Collaborator Author

@678098 678098 Jul 23, 2024

Choose a reason for hiding this comment

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

https://bloomberg.github.io/bde-resources/doxygen/bde_api_prod/group__bslstl__unorderedmap.html#ga010b0fbb7737d2e3a902daacda105193

From the bsl::unordered_map docs:

propagate to this object the allocator of rhs if the ALLOCATOR type has trait propagate_on_container_copy_assignment

The usual assignment operator might propagate the allocator and might not. Here, I make sure to construct the unordered map with the allocator from rhs.

Copy link
Collaborator

@hallfox hallfox Jul 30, 2024

Choose a reason for hiding this comment

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

bsl::allocator, which the map uses, doesn't allow this propagation: https://bloomberg.github.io/bde-resources/doxygen/bde_api_prod/group__bslma__stdallocator.html

I also don't think we should be rhs's allocator in this case, if allocator propagation were to happen on copy construction the class would handle it itself. Regardless I don't think we want to grab rhs's allocator this way. From the properties of [UsesBslmaAllocator trait docs](https://bloomberg.github.io/bde-resources/doxygen/bde_api_prod/group__bslma__usesbslmaallocator.html):

  • The allocator used by an object is not changed after construction (e.g., the assignment operator does not change the allocator used by an object).

So what I think we really want here is to use this's allocator to do each of these allocation operations.

@678098 678098 marked this pull request as ready for review July 23, 2024 17:08
@678098 678098 requested a review from a team as a code owner July 23, 2024 17:08
@678098 678098 requested a review from hallfox July 23, 2024 17:08
678098 added 3 commits August 13, 2024 13:37
Signed-off-by: Evgeny Malygin <emalygin@bloomberg.net>
Signed-off-by: Evgeny Malygin <emalygin@bloomberg.net>
Signed-off-by: Evgeny Malygin <emalygin@bloomberg.net>
@678098 678098 force-pushed the 240723_bmqt_QueueOptions_rule_of_3 branch from f67ee24 to bd8689a Compare August 13, 2024 12:41
Copy link
Collaborator

@hallfox hallfox left a comment

Choose a reason for hiding this comment

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

lgtm

@678098 678098 merged commit c8a640e into bloomberg:main Aug 13, 2024
29 checks passed
@678098 678098 deleted the 240723_bmqt_QueueOptions_rule_of_3 branch August 13, 2024 16:29
alexander-e1off pushed a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024
)


Signed-off-by: Evgeny Malygin <emalygin@bloomberg.net>
alexander-e1off pushed a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024
)


Signed-off-by: Evgeny Malygin <emalygin@bloomberg.net>
alexander-e1off pushed a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024
)


Signed-off-by: Evgeny Malygin <emalygin@bloomberg.net>
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.

2 participants