-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Implement LWG-4242 ranges::distance
does not work with volatile iterators
#5603
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
base: main
Are you sure you want to change the base?
Conversation
stl/inc/xutility
Outdated
template <class _It, sized_sentinel_for<decay_t<_It>> _Se> | ||
requires _Additionally_subtractable_for_distance<const _Se, _It> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constraint sized_sentinel_for<decay_t<_It>>
seems wrong for volatile
iterators. IIUC even when sized_sentinel_for<_Se, decay_t<_It>>
is modeled, correct behavior for volatile
version of _It
is not yet required.
Also, the const
for _Se
becomes questionable when volatile
-qualified _It
comes into the picture - as sized_sentinel_for<_Se, decay_t<_It>>
isn't requiring anything about volatile
-qualified _It
.
I guess it's better to replace decay_t<_It>
with another type which preserves volatile
for non-array _It
. CC @hewillk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why you introduced _additionally_subtractable_for_distance
. Could you provide an actual example of it having an effect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why you introduced
_additionally_subtractable_for_distance
. Could you provide an actual example of it having an effect?
See the test case - as the explicit constraints uses decay_t
, they would be satisfied by volatile reverse_iterator<int*>&
and reverse_iterator<int*>
. Without _Additionally_subtractable_for_distance
there will still be hard error. I believe "Effects: Equivalent to:" doesn't allow such a hard error and actually requires the implementation to add additional constraints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a LWG to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really comfortable with the amount of additional code being introduced here, even if you have an argument for why it's necessary. When I see "Effects: Equivalent to" in Standardese, I want our implementation to Do What The Standard Says, unless there's a simple reason why doing something different is better and it is an unquestionably lossless transformation. The reasoning involved here is too complicated for my cat-sized brain.
The main complicating factor that I see is our strengthened noexcept, but that could be handled by our _Choice_t
strategy technique. So, I am requesting:
- Let's implement the "Effects: Equivalent to" as depicted by the LWG resolution here.
- With
_Choice_t
if that's what's needed to preserve the noexcept strengthening. - File an LWG issue if you think that other cases still need to be fixed in the Standard.
The Standard should depict a specification that handles the cases we want it to handle, without requiring implementers to prove the Riemann Hypothesis before figuring out what code they actually need to write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can introduce the following exposition-only alias template:
template<class T>
using distance-iterator-t = // exposition only
conditional_t<is_array_v<remove_reference_t<T>>,
decay_t<T>, remove_const_t<remove_reference_t<T>>>;
And then modify this overload as:
template<class I, sized_sentinel_for<distance-iterator-t<I>> S>
constexpr iter_difference_t<distance-iterator-t<I>> ranges::distance(I&& first, S last);
@hewillk @StephanTLavavej What do you think?
045d2b8
to
370de5f
Compare
370de5f
to
4302333
Compare
stl/inc/xutility
Outdated
template <class _It, sized_sentinel_for<decay_t<_It>> _Se> | ||
requires _Additionally_subtractable_for_distance<const _Se, _It> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really comfortable with the amount of additional code being introduced here, even if you have an argument for why it's necessary. When I see "Effects: Equivalent to" in Standardese, I want our implementation to Do What The Standard Says, unless there's a simple reason why doing something different is better and it is an unquestionably lossless transformation. The reasoning involved here is too complicated for my cat-sized brain.
The main complicating factor that I see is our strengthened noexcept, but that could be handled by our _Choice_t
strategy technique. So, I am requesting:
- Let's implement the "Effects: Equivalent to" as depicted by the LWG resolution here.
- With
_Choice_t
if that's what's needed to preserve the noexcept strengthening. - File an LWG issue if you think that other cases still need to be fixed in the Standard.
The Standard should depict a specification that handles the cases we want it to handle, without requiring implementers to prove the Riemann Hypothesis before figuring out what code they actually need to write.
Per LWG-4242/WG21-P3742R0 (alternative link).
Fixes #5623.
This PR makes the involved overload of
ranges::distance
more constrained than explicitly written, because the "Effects: Equivalent to:" in [range.iter.op.distance]/3 isn't changed and we should propagate the implicit constraints per [structure.specifications]/4.Drive-by change:
ranges::distance
no longer performs such a conversion.