Make operator* and operator-> const in all iterators. #92
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
February 25, 2023 13:04
C++ iterators basically model pointers. Hence a const pointer can still be dereferenced to a mutable object (`T * const` *not* `const T*`). This is not true for many iterators in itertools since they only have non-`const` versions of `operator*`. This also violates C++ iterator concepts, see [the github issue](ryanhaining#91). This change basically replaces all non-`const` dereference operators with `const` ones. This was straight forward in most cases: - Some iterators own the data they return (note: that probably violates [`LegacyForwardIterator`](https://en.cppreference.com/w/cpp/named_req/ForwardIterator)). So those data fields were changed to `mutable`. - `GroupBy` advances the group while dereferencing. This does not work when the iterator is constant. Moved the advancing to the constructor and increment operators. This is the only real behavior change, please review carefully.
Just specializing on `T*` misses cv-qualified pointer types like `int *const`. This change uses `std::is_pointer_v` instead to determine whether a type is a pointer.
ryanhaining
requested changes
Mar 13, 2023
There was a problem hiding this comment.
thanks for working on this
I'm not sure how you formatted but clang-format -i --style=file $filename will match the existing style.
As-written I can't merge this without breaking existing uses because if anyone's sub_iter_ only has a non-const operator* then it breaks. I know that's arguably the fault of the caller, but we can't break them, and those uses do exist.
I believe the combinatoric ones can be changed to return const references rather than holding a mutable member.
groupby is the most complex tool and I will need more time to convince myself everything is okay with this change
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
C++ iterators basically model pointers. Hence a const pointer can still be dereferenced to a mutable object (
T * constnotconst T*). This is not true for many iterators in itertools since they only have non-constversions ofoperator*. This also violates C++ iterator concepts, see the github issue.This change basically replaces all non-
constdereference operators withconstones. This was straight forward in most cases:LegacyForwardIterator). So those data fields were changed tomutable.GroupByadvances the group while dereferencing. This does not work when the iterator is constant. Moved the advancing to the constructor and increment operators. This is the only real behavior change, please review carefully.Fixes #91 .