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 Subscription rule-of-three complaint #370

Merged

Conversation

678098
Copy link
Collaborator

@678098 678098 commented Jul 23, 2024

warning: implicitly-declared ‘BloombergLP::bmqt::Subscription& BloombergLP::bmqt::Subscription::operator=(const BloombergLP::bmqt::Subscription&)’ is deprecated [-Wdeprecated-copy]

Removes ~400 lines from the build log

Compliant to https://en.cppreference.com/w/cpp/language/rule_of_three

@678098 678098 marked this pull request as ready for review July 23, 2024 13:51
@678098 678098 requested a review from a team as a code owner July 23, 2024 13:51
@678098 678098 requested a review from dorjesinpo July 23, 2024 13:52
dorjesinpo
dorjesinpo previously approved these changes Jul 23, 2024
Copy link
Collaborator

@dorjesinpo dorjesinpo left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this.

src/groups/bmq/bmqt/bmqt_subscription.h Outdated Show resolved Hide resolved
Comment on lines 212 to 218
#if defined(BSLS_COMPILERFEATURES_SUPPORT_RVALUE_REFERENCES) && \
defined(BSLS_COMPILERFEATURES_SUPPORT_NOEXCEPT)
/// Assign to this object the value of the specified `rhs` object.
/// After performing this action, the `rhs` object will be left in a
/// valid, but unspecified state.
Subscription& operator=(Subscription&& rhs);
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably use bslmf::MovableRef instead of preprocessor guards here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On the other hand, what do you think about removing explicit copy constructor and assignment at all? It should deal with the warnings, and also we re-implemented the default one anyway. We don't use allocators for these classes, so we don't need to implement allocator-aware assignment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The proposed change is in the latest commit

Copy link
Collaborator

Choose a reason for hiding this comment

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

This would also work, for some reason I thought there was an explicit destructor here that was forcing us into a rule-of-3/rule-of-5 situation

Signed-off-by: Evgeny Malygin <emalygin@bloomberg.net>
@678098 678098 force-pushed the 240723_cleanup_implicit_bmqt_Subscription branch from ca40d2d to e309272 Compare July 23, 2024 14:40
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.

See https://bloomberg.github.io/bde-resources/doxygen/bde_api_prod/classbslmf_1_1MovableRef.html for how we can rewrite this to have rvalue references work on platforms with >=C++11 while also having fake moves on C++03

Signed-off-by: Evgeny Malygin <emalygin@bloomberg.net>
@678098 678098 changed the title Refactor[BMQT]: turn copy operator for Subscription explicit Refactor[BMQT]: make Subscription rule-of-three complaint Jul 23, 2024
@678098 678098 merged commit 3831d54 into bloomberg:main Jul 23, 2024
39 checks passed
@678098 678098 deleted the 240723_cleanup_implicit_bmqt_Subscription branch July 23, 2024 16:01
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.

3 participants