[libcxx-commits] [PATCH] D120180: [libc++][ranges] Implement changes to reverse_iterator from One Ranges Proposal.
Konstantin Varlamov via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Mar 15 13:42:48 PDT 2022
var-const added inline comments.
================
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.");
----------------
Quuxplusone wrote:
> 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? ;)
> Notice that your new tests are solidly in IFNDR-land //right now//
Strengthening the requirement by removing the `__is_cpp17_bidirectional_iterator` part (so that it's just `bidirectional_iterator`), only 4 tests fail out of 36. I think the cleanup shouldn't be too time consuming if it becomes necessary.
================
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; };
+
----------------
Quuxplusone wrote:
> 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.
Added some tests for `operator<=>`.
================
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), "");
+ }
----------------
Quuxplusone wrote:
> 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
It doesn't satisfy the requirement that the iterator given to `reverse_iterator` is at least a bidirectional iterator.
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