[libcxx-commits] [PATCH] D120180: [libc++][ranges] Implement changes to reverse_iterator from One Ranges Proposal.

Mon Apr 10 06:38:19 PDT 2023

smeenai 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.");
----------------
var-const wrote:
> 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.
I'm wading into this debate a year later :) I'm in the process of upgrading our libc++-using codebase to C++20, and I ran into this static_assert from https://github.com/facebook/folly/blob/8c52d79a616f7a89095d3121a7f02394c16c0848/folly/DynamicConverter.h#L88-L95 being instantiated with `std::unordered_map`. In the context of template metaprogramming, what's the best way to detect that a `reverse_iterator` can be constructed from an iterator before attempting to construct it, to avoid the static_assert? I could duplicate the check in the static_assert itself, of course, but I assume `__is_cpp17_bidirectional_iterator` isn't something I should be using outside the library, and just checking the other condition might be too restrictive.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120180/new/

https://reviews.llvm.org/D120180