[libc++] std::prev(it) should not compile for a non-bidi iterator #109456
Comments
ldionne
added
the
libc++
|
I think it would be better to static_assert here, since this isn't a constraint. |
|
There's LWG3197 for this, which is still open now. |
Per the LWG issue mentioned by @frederick-vs-ja , it seems like it's not clear to everyone whether there is a constraint or not. IMO this should definitely be a constraint, |
|
CC @jwakely perhaps we want to bring back that LWG issue to the table? |
|
It's been on the table for five years 😄 apart from the note I added a few months ago nobody had made a case for why we should prefer any of the options. |
|
I would argue for option (A) or option (C) in LWG3197, otherwise the result is that calling (That's me trying to make a case BTW 🙂) |
The std::prev function appeared to work on non Cpp17BidirectionalIterators, however it
behaved strangely by being the identity function. That was extremely misleading and
potentially dangerous, since several recent iterators that are C++20 bidirectional_iterators
don't satisfy the Cpp17BidirectionalIterator named requirement. For example:
auto zv = std::views::zip(vec1, vec2);
auto it = zv.begin() + 5;
auto it2 = std::prev(it); // "it2" will be the same as "it", instead of the previous one
Here, zip_view::iterator is a c++20 random_access_iterator, but it only satisfies the
Cpp17InputIterator named requirement because its reference type is a proxy type. Hence
`std::prev` would silently accept that iterator but it would do a no-op instead of going
to the previous position.
This patch changes `std::prev(it)` to produce an error at compile-time when instantiated
with a type that is not a Cpp17BidirectionalIterator.
Fixes #109456
The std::prev function appeared to work on non Cpp17BidirectionalIterators, however it
behaved strangely by being the identity function. That was extremely misleading and
potentially dangerous, since several recent iterators that are C++20 bidirectional_iterators
don't satisfy the Cpp17BidirectionalIterator named requirement. For example:
auto zv = std::views::zip(vec1, vec2);
auto it = zv.begin() + 5;
auto it2 = std::prev(it); // "it2" will be the same as "it", instead of the previous one
Here, zip_view::iterator is a c++20 random_access_iterator, but it only satisfies the
Cpp17InputIterator named requirement because its reference type is a proxy type. Hence
`std::prev` would silently accept that iterator but it would do a no-op instead of going
to the previous position.
This patch changes `std::prev(it)` to produce an error at compile-time when instantiated
with a type that is not a Cpp17BidirectionalIterator.
Fixes llvm#109456
std::prev(it, n)could conceivably be well-formed for a non-bidirectionalitifnis negative, howeverstd::prev(it)is always UB, and we can easily diagnose it at compile-time. Right nowstd::prev(non_bidi)is a no-op, which is absolutely vexing.https://godbolt.org/z/zaG3jTEqP
Potential patch:
The text was updated successfully, but these errors were encountered: