[libcxx-commits] [PATCH] D120180: [libc++][ranges] Implement changes to reverse_iterator from One Ranges Proposal.
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Mar 10 07:47:39 PST 2022
ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.
Looks really good, thanks! I have some comments so I'm requesting changes, but there's nothing major.
================
Comment at: libcxx/include/__iterator/reverse_iterator.h:54
+ using iterator_category = _If<__is_cpp17_random_access_iterator<_Iter>::value,
+ random_access_iterator_tag,
----------------
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?
================
Comment at: libcxx/include/__iterator/reverse_iterator.h:128
+#if _LIBCPP_STD_VER > 17
+ _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
+ pointer operator->() const
----------------
This can be made unconditionally `constexpr` now.
================
Comment at: libcxx/include/__iterator/reverse_iterator.h:130
+ pointer operator->() const
+#if !defined(_LIBCPP_HAS_NO_CONCEPTS)
+ requires is_pointer_v<_Iter> || requires(const _Iter i) { i.operator->(); }
----------------
You should be able to replace all instances of `_LIBCPP_HAS_NO_CONCEPTS` by `_LIBCPP_STD_VER > 17` now that we've bumped the compilers.
For this specific instance, it means it can be eliminated altogether.
================
Comment at: libcxx/include/__iterator/reverse_iterator.h:134-137
+ _Iter __tmp = current;
+ --__tmp;
+ if constexpr (is_pointer_v<_Iter>) {
+ return __tmp;
----------------
Any reason for not using verbatim what's in the standard, i.e. `prev(current)`?
================
Comment at: libcxx/include/__iterator/reverse_iterator.h:150-173
+ reverse_iterator& operator++() { --current; return *this; }
+
_LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
- reverse_iterator operator++(int) {reverse_iterator __tmp(*this); --current; return __tmp;}
+ reverse_iterator operator++(int) { reverse_iterator __tmp(*this); --current; return __tmp; }
+
_LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
+ reverse_iterator& operator--() { ++current; return *this; }
----------------
Suggestion: land these NFCs in a separate commit, no need to review or anything. That'll clean up this diff.
================
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:
> > > 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.
================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/iter_move.pass.cpp:64
+
+int main(int, char**) {
+ // Can use `iter_move` with a regular array.
----------------
Is there a reason why this isn't in a `constexpr` function and then called as `test(); static_assert(test(), "");` so that we test it both at runtime and at compile-time?
================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/iter_move.pass.cpp:109
+ static_assert(!std::is_nothrow_copy_constructible_v<ThrowingCopyNoexceptDecrement>);
+ ASSERT_NOEXCEPT(std::ranges::iter_move(--std::declval<ThrowingCopyNoexceptDecrement&>()));
+ using RI = std::reverse_iterator<ThrowingCopyNoexceptDecrement>;
----------------
Thanks for being diligent on testing even the noexcept clauses. I'm glad this is the standard we hold ourselves to, even though I know those tests are tedious to write.
================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/iter_swap.pass.cpp:65
+
+int main(int, char**) {
+ // Can use `iter_swap` with a regular array.
----------------
The same question/comment about `constexpr` testing applies here too.
================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/types.pass.cpp:108
+
+int main(int, char**) {
+ test<bidirectional_iterator<char*> >();
----------------
Suggestion: move this test to a `.compile.pass.cpp` test and rename `main` to something else to remove the impression that we're running anything.
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