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.

Sign up for GitHub

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

Jump to bottom

dereference operators should be const #91

Open
thomas001 opened this issue Feb 23, 2023 · 0 comments · May be fixed by #92
Open

dereference operators should be const #91

thomas001 opened this issue Feb 23, 2023 · 0 comments · May be fixed by #92

Comments

@thomas001
Copy link

Currently a lot iterators have only non-const operator* or operator-> (for example in groupby).

This is not correct according to the legacy and C++20 iterator concepts:

  • LegacyInputIterator states that *i and i->m should be valid expressions given

    i and j, values of type It or const It

    Note the inclusion of const It here.

  • std::indirectly_readable (which is required by std::input_iterator) has

    requires(const In in) {
  // ...
  { *in } -> std::same_as<std::iter_reference_t<In>>;
  // ....
} &&

    Note that the concept requires *in vor values of const In.

Since iterators are copyable, one can always work around this by copying a const itertools iterator and dereferencing the copy. Yet, this might be inefficient for some iterators. Also, itertools iterators not implementing c++20 iterator concepts will probably make them much less useful in the future. Finally, there are other range and iterator adapter libraries out there which might assume a const iterator is dereferenceable.

For some iterators in itertools the fix seems to be straightforward by adding const to the operators. Others, like groupby, seem to require more changes.
thomas001 pushed a commit to thomas001/cppitertools that referenced this issue Feb 25, 2023
Make operator* and operator-> const in all iterators.
242cc42 
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.
thomas001 pushed a commit to thomas001/cppitertools that referenced this issue Feb 25, 2023
Make operator* and operator-> const in all iterators.
9dd7b0d 
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.
thomas001 pushed a commit to thomas001/cppitertools that referenced this issue Feb 25, 2023
Make operator* and operator-> const in all iterators.
568db64 
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.
@thomas001 thomas001 linked a pull request Feb 25, 2023 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make operator* and operator-> const in all iterators. thomas001/cppitertools
1 participant
@thomas001