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[MWC]: explicit operator= in mwcc containers #372

Merged
merged 2 commits into from
Jul 24, 2024

Conversation

678098
Copy link
Collaborator

@678098 678098 commented Jul 23, 2024

Removing the custom copy constructor OrderedHashMap_SequentialIterator(const NcIter& other); doesn't work since it produces compilation errors

In file included from /blazingmq/src/groups/bmq/bmqimp/bmqimp_brokersession.cpp:17:
In file included from /blazingmq/src/groups/bmq/bmqimp/bmqimp_brokersession.h:37:
In file included from /blazingmq/src/groups/bmq/bmqimp/bmqimp_messagecorrelationidcontainer.h:39:
/blazingmq/src/groups/bmq/bmqp/bmqp_requestmanager.h:1509:30: error: no viable conversion from 'OrderedHashMap_SequentialIterator<pair<...>>' to 'OrderedHashMap_SequentialIterator<const pair<...>>'
    for (RequestMapConstIter it = requestsCopy.begin();
                             ^    ~~~~~~~~~~~~~~~~~~~~

Implemented explicit operator= for OrderedHashMap_SequentialIterator and OrderedHashMapWithHistory_Iterator

@678098 678098 force-pushed the 240723_mwcc_iterators_warnings branch from 91a2d9b to a740c28 Compare July 23, 2024 17:13
@678098 678098 changed the title Refactor[MWC]: explicit operator= in OrderedHashMap_SequentialIterator Refactor[MWC]: explicit operator= in mwcc containers Jul 23, 2024
@678098 678098 marked this pull request as ready for review July 23, 2024 18:06
@678098 678098 requested a review from a team as a code owner July 23, 2024 18:06
inline OrderedHashMap_SequentialIterator<VALUE>&
OrderedHashMap_SequentialIterator<VALUE>::operator=(const NcIter& rhs)
{
d_link_p = rhs.d_link_p;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that I decided to not to add checks for self-assignment, since we don't assign many fields, and just assign will be much faster than if-branch.

We don't check for self-assignment for some small classes, for example

template <class VALUE>
inline WeakMemFnResult<VALUE>&
WeakMemFnResult<VALUE>::operator=(const WeakMemFnResult& original)
{
    d_value.object() = original.d_value.object();
    return *this;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

This makes sense, this is such a cheap object that we shouldn't worry about self-assignment.

@678098 678098 force-pushed the 240723_mwcc_iterators_warnings branch from 33c9bc0 to 4376a60 Compare July 24, 2024 12:52
@ddragan-bloomberg
Copy link
Contributor

From what I gathered the OrderedHashMap_SequentialIterator& operator=(const NcIter&) definition exists so that a const iterator is constructible from a non-const itertator, but the same definition also might act as the copy constructor. Took me a while to figure this out. I think this is convoluted. A better way would be to:

  • not define the copy constructor, not the copy operator;
  • define a special from-const-iterator constructor.

@678098
Copy link
Collaborator Author

678098 commented Jul 24, 2024

  • not define the copy constructor, not the copy operator;
  • define a special from-const-iterator constructor.

If I remove the original OrderedHashMapWithHistory_Iterator(const NcIter& other); copy constructor, I will get compilation errors:

blazingmq/src/groups/mwc/mwcc/mwcc_orderedhashmapwithhistory.h:698:12: error: no matching conversion for functional-style cast from 'const iterator' (aka 'const OrderedHashMapWithHistory_Iterator<Value>') to 'const_iterator' (aka 'OrderedHashMapWithHistory_Iterator<const Value>')
    return const_iterator(d_first);

These errors will remain even if I define from-const-iterator constructor

typedef typename bsl::add_const<VALUE>::type      CType;
typedef OrderedHashMapWithHistory_Iterator<CType> CIter;
OrderedHashMapWithHistory_Iterator(const CIter& other);

@ddragan-bloomberg
Copy link
Contributor

I just realized that the assignment operator is also double-purpose. It allows to assign a non-const iterator to a const one, and services as the copy-assignment operator. So it should be explicitly declared and implemented.

-- this code is fine

Copy link
Collaborator

@pniedzielski pniedzielski left a comment

Choose a reason for hiding this comment

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

Small nit, if you want to fix, go ahead and I'll reapprove; otherwise looks good.

@@ -296,6 +296,9 @@ class OrderedHashMap_SequentialIterator {

// MANIPULATORS

/// Assign to this object the value of the specified `rhs` object.
OrderedHashMap_SequentialIterator& operator=(const NcIter&);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Because of the documentation comment, I think we want an rhs name here (like in the other commit in this PR):
OrderedHashMap_SequentialIterator& operator=(const NcIter& rhs);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I already force-pushed a fix for this. Might be GH lagging

Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh, you're right! Looks good, go ahead and merge.

inline OrderedHashMap_SequentialIterator<VALUE>&
OrderedHashMap_SequentialIterator<VALUE>::operator=(const NcIter& rhs)
{
d_link_p = rhs.d_link_p;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This makes sense, this is such a cheap object that we shouldn't worry about self-assignment.

@678098 678098 merged commit ecca25c into bloomberg:main Jul 24, 2024
39 checks passed
@678098 678098 deleted the 240723_mwcc_iterators_warnings branch July 24, 2024 20:09
alexander-e1off pushed a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024
* Refactor[MWC]: explicit operator= in OrderedHashMap_SequentialIterator

* Refactor[MWC]: explicit operator= in OrderedHashMapWithHistory_Iterator

Signed-off-by: Evgeny Malygin <[email protected]>
alexander-e1off pushed a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024
* Refactor[MWC]: explicit operator= in OrderedHashMap_SequentialIterator

* Refactor[MWC]: explicit operator= in OrderedHashMapWithHistory_Iterator

Signed-off-by: Evgeny Malygin <[email protected]>
alexander-e1off pushed a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024
* Refactor[MWC]: explicit operator= in OrderedHashMap_SequentialIterator

* Refactor[MWC]: explicit operator= in OrderedHashMapWithHistory_Iterator

Signed-off-by: Evgeny Malygin <[email protected]>
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