Skip to content

COMP: Improve const correctness for ValueIterators #1056

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

Merged
merged 1 commit into from
Oct 17, 2019

Conversation

hjmjohnson
Copy link
Contributor

The protected deref method had inconsistent interface
of being a const function that returned a non-const
reference. Resolves #914.

The protected deref method had inconsistent interface
of being a const function that returned a non-const
reference.  Resolves open-source-parsers#914.
@baylesj baylesj merged commit b082693 into open-source-parsers:master Oct 17, 2019
* because the returned references/pointers can be used
* to change state of the base class.
*/
reference operator*() { return deref(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically this broke binary compatibility, so this should have been a major version bump.

I guess we get away with this because probably nobody uses this method. If nobody complains, we can keep it.

@BillyDonahue
Copy link
Contributor

I have to argue with the original premise of this change.

I did make the argument in the thread for issue #914 but received no response.

#914 (comment)

Iterators should model pointer. All standard container itererators do as well.

All 4 of these possiblities exist:

    T*
    const T*
    T*const
    const T*const

The constness of the pointer is independent of the constness of the thing to which it points.
Same with iterators. deref() should continue to be const method of iterator.

The right signatures are:

Value& ValueIterator::deref() const;
const Value& ConstValueIterator::deref() const;

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.

Issues compiling with uClibc++
4 participants