[libcxx-commits] [PATCH] D120180: [libc++][ranges] Implement changes to reverse_iterator from One Ranges Proposal.
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Fri Mar 11 08:17:26 PST 2022
Quuxplusone added inline comments.
================
Comment at: libcxx/include/__iterator/reverse_iterator.h:54
+ using iterator_category = _If<__is_cpp17_random_access_iterator<_Iter>::value,
+ random_access_iterator_tag,
----------------
var-const wrote:
> ldionne wrote:
> > The intent of this change is so that a `reverse_iterator<contiguous-iterator>` advertises itself as a `random_access_iterator`, not a `contiguous_iterator`. Right?
> I think so (it definitely has this effect), but I would have to do some archaeology to find out if that was the main intention (the One Ranges Proposal omits any rationale). Please let me know if you'd like me to confirm.
I'd explain it as: We know a `reverse_iterator` is always at least a bidirectional iterator. But //if// the `_Iter` type is random-access, //then// our `reverse_iterator` should also be random-access!
Your explanation adds: ''...But if the `_Iter` type is contiguous, then... nothing special. Just leave the `reverse_iterator` as random-access in that case." Which is absolutely correct, but it's kind of the boring part of the explanation, IMHO. ;)
================
Comment at: libcxx/include/__iterator/reverse_iterator.h:134-137
+ _Iter __tmp = current;
+ --__tmp;
+ if constexpr (is_pointer_v<_Iter>) {
+ return __tmp;
----------------
var-const wrote:
> ldionne wrote:
> > Any reason for not using verbatim what's in the standard, i.e. `prev(current)`?
> I don't think there was a particularly strong reason. Changed to the Standard version.
@var-const: AFAIK, this should be `std::prev`, not `ranges::prev`. Please use `std::prev`. I'm not sure the difference is detectable, but if you can think of some way it //is// detectable then please add a regression test too.
================
Comment at: libcxx/include/__iterator/reverse_iterator.h:188-190
+ noexcept(is_nothrow_copy_constructible_v<_Iter> &&
+ is_nothrow_copy_constructible_v<_Iter2> &&
+ noexcept(ranges::iter_swap(--declval<_Iter&>(), --declval<_Iter2&>()))) {
----------------
var-const wrote:
> Quuxplusone wrote:
> > Btw, the difference between
> > ```
> > noexcept(ranges::iter_swap(--declval<_Iter&>(), --declval<_Iter2&>()))
> > ```
> > and
> > ```
> > noexcept(ranges::iter_swap(--__x, --__y))
> > ```
> > is subtle. Do you have a test that will fail if someone makes that substitution? (If not, please add one.)
> Hmm, but `__x` and `__y` are const references, so calling `--` on them should be invalid, or at least unreasonable? Also, trying to make the change as-is (without adding a new test), I get:
> ```
> In file included from libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/iter_swap.pass.cpp:20:
> In file included from build/include/c++/v1/iterator:642:
> In file included from build/include/c++/v1/__iterator/common_iterator.h:18:
> build/include/c++/v1/__iterator/iter_swap.h:41:7: error: exception specification of 'iter_swap<int *>' uses itself
> iter_swap(_VSTD::forward<_T1>(__x), _VSTD::forward<_T2>(__y));
> ^
> build/include/c++/v1/__iterator/iter_swap.h:41:7: note: in instantiation of exception specification for 'iter_swap<int *>' requested here
> build/include/c++/v1/__iterator/iter_swap.h:41:7: note: in instantiation of requirement here
> iter_swap(_VSTD::forward<_T1>(__x), _VSTD::forward<_T2>(__y));
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> build/include/c++/v1/__iterator/iter_swap.h:40:5: note: while substituting template arguments into constraint expression here
> requires (_T1&& __x, _T2&& __y) {
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> build/include/c++/v1/__iterator/iter_swap.h:51:16: note: while checking the satisfaction of concept '__unqualified_iter_swap<const std::reverse_iterator<int *> &, const std::reverse_iterator<int *> &>' requested here
> requires __unqualified_iter_swap<_T1, _T2>
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> build/include/c++/v1/__iterator/iter_swap.h:51:16: note: while substituting template arguments into constraint expression here
> requires __unqualified_iter_swap<_T1, _T2>
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> build/include/c++/v1/__iterator/reverse_iterator.h:190:20: note: while checking constraint satisfaction for template 'operator()<const std::reverse_iterator<int *> &, const std::reverse_iterator<int *> &>' required here
> noexcept(ranges::iter_swap(__x, __y))) {
> ^~~~~~
> build/include/c++/v1/__iterator/reverse_iterator.h:190:20: note: in instantiation of function template specialization 'std::ranges::__iter_swap::__fn::operator()<const std::reverse_iterator<int *> &, const std::reverse_it
> erator<int *> &>' requested here
> libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/iter_swap.pass.cpp:76:41: note: in instantiation of exception specification for 'iter_swap<int *>' requested here
> static_assert(std::same_as<decltype(iter_swap(rb, re)), void>);
> ^
> ```
>
> Do you think it's worth digging further, given this?
> Do you think it's worth digging further, given this?
For the record, no, that's fine. :)
================
Comment at: libcxx/include/__iterator/reverse_iterator.h:49
+#if !defined(_LIBCPP_HAS_NO_CONCEPTS)
+ static_assert(__is_cpp17_bidirectional_iterator<_Iter>::value || bidirectional_iterator<_Iter>, "The Iterator "
+ "template argument must be a bidirectional iterator.");
----------------
var-const wrote:
> ldionne wrote:
> > var-const wrote:
> > > Quuxplusone wrote:
> > > > var-const wrote:
> > > > > Quuxplusone wrote:
> > > > > > var-const wrote:
> > > > > > > http://eel.is/c++draft/reverse.iterators#reverse.iter.requirements-1:
> > > > > > >
> > > > > > > > "The template parameter `Iterator` shall either meet the requirements of a `Cpp17BidirectionalIterator` (`[bidirectional.iterators]`) or model `bidirectional_iterator` (`[iterator.concept.bidir]`)."
> > > > > > >
> > > > > > > My understanding is that this is IFNDR, but adding a check seems like it could prevent harder-to-read errors during instantiation.
> > > > > > >
> > > > > > > There's a similar requirement for random access iterators:
> > > > > > >
> > > > > > > > "Additionally, `Iterator` shall either meet the requirements of a `Cpp17RandomAccessIterator` (`[random.access.iterators]`) or model `random_access_iterator` (`[iterator.concept.random.access]`) if the definitions of any of the members `operator+`, `operator-`, `operator+=`, `operator-=` (`[reverse.iter.nav]`), or `operator[]` (`[reverse.iter.elem]`), or the non-member operators (`[reverse.iter.cmp]`) `operator<`, `operator>`, `operator<=`, `operator>=`, `operator-`, or `operator+` (`[reverse.iter.nonmember]`) are instantiated (`[temp.inst]`)."
> > > > > > >
> > > > > > > I'm not sure that can be done with a `static_assert` -- I tried placing it into the bodies of the mentioned operators, and the compiler produces errors trying to instantiate non-existing functions before it evaluates the `static_assert`.
> > > > > > clang-format strikes again, in the formatting here; please fix it up somehow.
> > > > > > I'm ambivalent on adding this `static_assert`. It will probably //worsen// the error messages, actually, since "such-and-such operator doesn't exist when I tried to use it" is generally much easier to deal with than "this random static_assert was false instead of true." Have you actually looked at the error messages for e.g. `reverse_iterator<std::forward_list<int>::iterator>`, and //do// you think this is an improvement?
> > > > > > Here's the state of the art, for comparison: https://godbolt.org/z/nne1Mvzan
> > > > > > ```
> > > > > > __iterator/reverse_iterator.h:120:37: error: cannot decrement value of type 'std::__forward_list_iterator<std::__forward_list_node<int, void *> *>'
> > > > > > reverse_iterator& operator++() {--current; return *this;}
> > > > > > ^ ~~~~~~~
> > > > > > ```
> > > > > > I predict you'll find that this static_assert is not an improvement, and decide not to keep it.
> > > > > I'm actually formatting most of these manually (don't want to get used to the convenience of clang-format when there's a chance it's taken away :) ). Reformatted this bit with clang-format now, I think it's an improvement.
> > > > >
> > > > > Here's the full error I get with a static assert upon trying to create a reverse iterator from a forward iterator:
> > > > >
> > > > > ```
> > > > > In file included from /Users/varconst/work/llvm-project/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.default.pass.cpp:15:
> > > > > In file included from /Users/varconst/work/llvm-project/build/include/c++/v1/iterator:669:
> > > > > In file included from /Users/varconst/work/llvm-project/build/include/c++/v1/__iterator/reverse_access.h:14:
> > > > > /Users/varconst/work/llvm-project/build/include/c++/v1/__iterator/reverse_iterator.h:49:5: error: static_assert failed due to requirement '__is_cpp17_bidirectional_iterator<forward_iterator<int *>>::value || bidirectional_iterator<forward_iterator<int *>>' "The Iterator template argument must be a bidirectional iterator."
> > > > > static_assert(__is_cpp17_bidirectional_iterator<_Iter>::value || bidirectional_iterator<_Iter>,
> > > > > ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > /Users/varconst/work/llvm-project/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.default.pass.cpp:40:13: note: in instantiation of template class 'std::reverse_iterator<forward_iterator<int *>>' requested here
> > > > > BadIter i;
> > > > > ^
> > > > > 1 error generated.
> > > > > ```
> > > > >
> > > > > Compared to the baseline (where construction succeeds but e.g. trying to increment the iterator fails):
> > > > > ```
> > > > > In file included from /Users/varconst/work/llvm-project/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.default.pass.cpp:15:
> > > > > In file included from /Users/varconst/work/llvm-project/build/include/c++/v1/iterator:669:
> > > > > In file included from /Users/varconst/work/llvm-project/build/include/c++/v1/__iterator/reverse_access.h:14:
> > > > > /Users/varconst/work/llvm-project/build/include/c++/v1/__iterator/reverse_iterator.h:148:37: error: cannot decrement value of type 'forward_iterator<int *>'
> > > > > reverse_iterator& operator++() {--current; return *this;}
> > > > > ^ ~~~~~~~
> > > > > /Users/varconst/work/llvm-project/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.default.pass.cpp:42:5: note: in instantiation of member function 'std::reverse_iterator<forward_iterator<int *>>::operator++' requested here
> > > > > ++i;
> > > > > ^
> > > > > 1 error generated.
> > > > > ```
> > > > >
> > > > > For easier comparison, the same messages with all the cruft removed:
> > > > > ```
> > > > > __iterator/reverse_iterator.h:49:5: error: static_assert failed due to requirement '__is_cpp17_bidirectional_iterator<forward_iterator<int *>>::value || bidirectional_iterator<forward_iterator<int *>>' "The Iterator template argument must be a bidirectional iterator."
> > > > > static_assert(__is_cpp17_bidirectional_iterator<_Iter>::value || bidirectional_iterator<_Iter>,
> > > > > ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > ```
> > > > >
> > > > > ```
> > > > > __iterator/reverse_iterator.h:148:37: error: cannot decrement value of type 'forward_iterator<int *>'
> > > > > reverse_iterator& operator++() {--current; return *this;}
> > > > > ^ ~~~~~~~
> > > > > ```
> > > > >
> > > > > I do think that the static assertion improves the error message slightly. The error is caught early and the message explains the fundamental problem rather than a symptom of the problem. Error messages like the current one are often cited as one of the problems with C++ templates -- showing a technical detail in the instantiation. While it's not hard to figure out why a reverse iterator needs the decrement operator, it's still an additional mental step, and I think saying that the iterator has to be bidirectional is a better way to express the requirement than dealing with individual requirements of a bidirectional iterator.
> > > > Hm! I was hoping you'd quickly agree that
> > > > ```
> > > > --current;
> > > > ^ ~~~~~~~
> > > > ```
> > > > was obviously the better error message.
> > > > ```
> > > > static_assert(__is_cpp17_bidirectional_iterator<_Iter>::value || bidirectional_iterator<_Iter>,
> > > > ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > ```
> > > > sadly gives the programmer no information about how to fix the problem with their code. So that makes it a //worse// error message.
> > > >
> > > > I'll pull rank on this one. :) Please remove the `static_assert`.
> > > I think both are workable, so I'm ok with removing the `static_assert`, but I still feel that saying //why// it doesn't compile is more useful than saying //what// doesn't compile.
> > >
> > > > sadly gives the programmer no information about how to fix the problem with their code
> > > It leads the programmer in the right direction -- they can look up the `bidirectional_iterator` requirements or just do `static_assert(std::bidirectional_iterator<MySupposedlyBidirectionalIterator>)` to see what exactly is missing from their type (which might be more than just the `operator--`).
> > I have to say -- I think the `static_assert` approach is superior for a couple of reasons (in no order):
> >
> > - Instantiating `reverse_iterator` with a non-bidi iterator is a precondition type of error, per http://eel.is/c++draft/reverse.iterators#reverse.iter.requirements-1. What we normally do for these errors is assert, or `static_assert` when detectable at compile-time. So there's a consistency argument.
> > - A `static_assert` firing makes it clear that the user messed up. When a user sees a `static_assert` with a message we bothered writing for them, they immediately go "oh, I must have messed up", which provides a nicer experience than letting them scratch their head over "why is this code not compiling".
> > - We have a message we can use to make it arbitrarily clear what's going wrong. We shouldn't be shy to explain the issue properly so as to make the `static_assert` undoubtedly easier to understand than the compilation error.
> > - Without a `static_assert`, it's possible that some users would try to fix their code by just adding the missing operator to their iterator type, without necessarily making it a proper bidirectional iterator. So they'd still be in IFNDR-land, when we could instead have helped them understand the problem and fix their code.
> > - It is generally better to fail earlier rather than later. For example, we don't want users to use `reverse_iterator<non-bidi>` successfully until they finally try to use `operator++` on it, which would start failing. For instance, without the assertion, I think users could use `reverse_iterator<non-bidi>` successfully if they only used `operator--`, right? If so, that's not a good thing.
> >
> >
> > By the way, the above applies to anywhere in the library where we have ill-formed code that we're able to diagnose. If we agree on this, we can take it as a blanket policy to prefer `static_assert` over "lazy instantiation errors" when we have those cases.
> >
> > So, all that being said, my vote would go for adding a `static_assert` with a nice error message, and adding a libc++ specific test to check that we do catch it.
> Restored the `static_assertion` and its test (`bad_template_argument.verify.cpp` in `libcxx` tests). Please let me know if you'd like to expand the error message.
>
> One thing to mention, though -- the Standard's constraints are:
> > The template parameter Iterator shall either meet the requirements of a `Cpp17BidirectionalIterator` (`[bidirectional.iterators]`) or model `bidirectional_iterator` (`[iterator.concept.bidir]`).
>
> I presumed that `__is_cpp17_bidirectional_iterator` implements `Cpp17BidirectionalIterator`. However, it seems significantly more lax -- if I'm reading the implementation correctly, it only checks the `iterator_category`, not the provided operations. So the current check is significantly weaker than it should be -- any type providing the expected `iterator_category` and the other 3-4 typedefs would do. Do you think I should fix this in this patch or a follow-up? Or perhaps create a local version of `__is_cpp17_bidirectional_iterator` in this file?
>
> > For instance, without the assertion, I think users could use `reverse_iterator<non-bidi>` successfully if they only used `operator--`, right? If so, that's not a good thing.
>
> I tried it out to verify, and yes, functions like `operator--` and `base` do work if there is no `static_assertion`.
FWIW, I still strongly disagree with this direction, on principle; but will defer to @ldionne.
> Without a static_assert, [...] just adding the missing operator to their iterator [...] they'd still be in IFNDR-land
True, if your goal is to keep people out of IFNDR-land entirely, then the static_assert is the right answer. I still think that for anything in "STL Classic", outside of `namespace ranges`, it would be better to keep the "Classic" wild-west template semantics so as to avoid breaking code unnecessarily... Basically, I //like// IFNDR-land. :)
> the current check is significantly weaker than it should be
FWIW IMHO, it's not worth creating a local version of anything for just this file; either strengthen `__is_cpp17_bidirectional_iterator` or do nothing. But if strengthening `__is_cpp17_bidirectional_iterator`, I'd again caution to avoid breaking existing code if possible. (Or at least avoid breaking it //without good reason//. For @ldionne, "staying out of IFNDR-land" might count as a good reason. For me it wouldn't. So that's why we end up on different sides of this issue.)
Notice that your new tests are solidly in IFNDR-land //right now//; Ctrl+F down for where you said "__is_cpp17_bidirectional_iterator is pretty lax." Are you volunteering yourself to clean up that test code now? ;)
================
Comment at: libcxx/test/libcxx/iterators/predef.iterators/reverse.iterators/bad_template_argument.fail.cpp:22-23
+ {
+ using BadIter = std::reverse_iterator<forward_iterator<int*>>;
+ BadIter i; //expected-error-re@*:* {{static_assert failed{{.*}} "The Iterator template argument must be a bidirectional iterator."}}
+ }
----------------
var-const wrote:
> Quuxplusone wrote:
> > I believe you wanted this to be `.verify.cpp`, not `.fail.cpp`. I think (but am not sure) that `.fail.cpp` just checks for //any// compiler failure, and doesn't actually respect `//expected-error` comments.
> >
> > However, I expect this to be moot when you decide to remove the static_assert again. :)
> I just checked and it behaves exactly the same (correctly) with either name. So probably the implementations of `verify` and `fail` converged at some point?
Aha, see libcxx/utils/libcxx/test/format.py:
```
FOO.verify.cpp - Compiles with clang-verify. This type of test is
automatically marked as UNSUPPORTED if the compiler
does not support Clang-verify.
FOO.fail.cpp - Compiled with clang-verify if clang-verify is
supported, and equivalent to a .compile.fail.cpp
test otherwise. This is supported only for backwards
compatibility with the test suite.
```
So, there's no difference, but for consistency please rename to .verify.cpp.
================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/sfinae.compile.pass.cpp:62
+template<class T> concept HasGreater = requires (T t) { t > t; };
+template<class T> concept HasGreaterOrEqual = requires (T t) { t >= t; };
+
----------------
var-const wrote:
> Quuxplusone wrote:
> > What about `operator<=>`?
> My intention was to check the `constraints` section of these operators ("Constraints: `x.base() <= y.base()` is well-formed and convertible to `bool`"). `operator<=>` has no `constraints` section -- unless it should be inferred implicitly somehow?
>
> By the way -- do you think it's worthwhile checking the return type as well? Basically, run all the same checks if the return type is `ConvertibleToBool` or `NotConvertibleToBool`? It seems like overkill, but happy to add the tests if you think it's worth testing.
> `operator<=>` has no constraints section -- unless it should be inferred implicitly somehow?
It has a `requires`-clause which expresses the same constraints. (A requires-clause makes it a //constrained template//, i.e., a template with constraints. :) The Standard just doesn't need to say the constraints in English because they're already said in code.)
> run all the same checks if the return type is ConvertibleToBool or NotConvertibleToBool?
Hmm, so it would be something like this instead?
```
struct B { explicit operator bool() const; };
struct NB { };
template<int Version>
struct NoEqualityCompIter : IterBase {
NB operator==(NoEqualityCompIter) const requires (Version == 1);
B operator!=(NoEqualityCompIter) const;
B operator<(NoEqualityCompIter) const;
B operator>(NoEqualityCompIter) const;
B operator<=(NoEqualityCompIter) const;
B operator>=(NoEqualityCompIter) const;
};
static_assert( HasEqual<std::reverse_iterator<int*>>);
static_assert(!HasEqual<std::reverse_iterator<NoEqualityCompIter<1>>>);
static_assert( HasNotEqual<std::reverse_iterator<NoEqualityCompIter<1>>>);
static_assert( HasLess<std::reverse_iterator<NoEqualityCompIter<1>>>);
static_assert( HasLessOrEqual<std::reverse_iterator<NoEqualityCompIter<1>>>);
static_assert( HasGreater<std::reverse_iterator<NoEqualityCompIter<1>>>);
static_assert(!HasEqual<std::reverse_iterator<NoEqualityCompIter<2>>>);
static_assert( HasNotEqual<std::reverse_iterator<NoEqualityCompIter<2>>>);
static_assert( HasLess<std::reverse_iterator<NoEqualityCompIter<2>>>);
static_assert( HasLessOrEqual<std::reverse_iterator<NoEqualityCompIter<2>>>);
static_assert( HasGreater<std::reverse_iterator<NoEqualityCompIter<2>>>);
```
That's not terrible but also not thrilling. I'm weakly against, but will leave it up to you and/or whoever has a stronger opinion.
================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/types.compile.pass.cpp:56-60
+ if constexpr (std::is_same_v<typename T::iterator_category, std::contiguous_iterator_tag>) {
+ static_assert((std::is_same<typename R::iterator_category, std::random_access_iterator_tag>::value), "");
+ } else {
+ static_assert((std::is_same<typename R::iterator_category, typename T::iterator_category>::value), "");
+ }
----------------
This metaprogramming was a yellow flag for me, especially after all our discussion about lax iterator requirements for legacy iterators. Could you take a look at this Godbolt and see if you agree that it's conforming C++ (i.e. `It` does literally satisfy the legacy requirements so we're not in IFNDR-land)? Does this PR make us pass this test case? Could you make sure this is tested somewhere?
https://godbolt.org/z/zsfYseKK6
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120180/new/
https://reviews.llvm.org/D120180
More information about the libcxx-commits
mailing list