[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
Mon Feb 28 04:45:57 PST 2022
var-const added inline comments.
================
Comment at: libcxx/include/__iterator/iterator_traits.h:145
+// The `cpp17-*-iterator` exposition-only concepts are easily confused with the Cpp17*Iterator tables, so the
+// exposition-only concepts have been banished to a namespace that makes it obvious they have a niche use-case.
namespace __iterator_traits_detail {
----------------
Quuxplusone wrote:
> var-const wrote:
> > Quuxplusone wrote:
> > > var-const wrote:
> > > > This is a drive-by change: I just found the original comment unclear.
> > > Sure. FWIW, I'd just remove this comment. It kinda seems like the only point of this comment is to explain something that the comment itself says is "obvious," which means it's like //self-referentially// bad. Plus, I don't even know what it's talking about. If "the Cpp17*Iterator tables" make an appearance in libc++'s codebase, I don't know where.
> > >
> > > Perhaps all this comment means is: "`__iterator_traits_detail::__cpp17_iterator` is not the same as `__is_cpp17_input_iterator`." But that really //is// obvious, and only serves to raise more questions, like "Wait, why isn't it?" So again, I think the solution is to delete the confusing comment.
> > I agree that the comment is still vague and confusing; it presumes the reader has a lot of context and leaves that context out. I think the most confusing part is "Cpp17*Iterator tables" (which is also the part you pointed out). Most likely, it refers to [`iterator.cpp17`](https://eel.is/c++draft/iterator.cpp17), which indeed contains some tables (though mentioning them here is at best unhelpful). I tweaked the wording a bit -- do you think it reads better now?
> It's //better// now, but I would still prefer if it were just gone.
I think it would only make sense to remove the comment if it were obvious from a glance. I find the situation not obvious -- somewhat confusing, actually -- so personally I find the comment helpful. If you have any other suggestions on how to improve the wording, please let me know.
================
Comment at: libcxx/include/__iterator/reverse_iterator.h:139-146
+ pointer operator->() const
+#if !defined(_LIBCPP_HAS_NO_CONCEPTS)
+ requires is_pointer_v<_Iter>
+ || requires(const _Iter i) { i.operator->(); }
+#endif
+ {
+ return _VSTD::addressof(operator*());
----------------
Quuxplusone wrote:
> var-const wrote:
> > Quuxplusone wrote:
> > > I think what I'd do here is
> > > ```
> > > #if _LIBCPP_STD_VER > 17
> > > pointer operator->() const
> > > #if !defined(_LIBCPP_HAS_NO_CONCEPTS)
> > > requires is_pointer_v<_Iter> || requires(const _Iter i) { i.operator->(); }
> > > #endif // !defined(_LIBCPP_HAS_NO_CONCEPTS)
> > > {
> > > _Iter __tmp = current;
> > > --__tmp;
> > > if constexpr (is_pointer_v<_Iter>) {
> > > return __tmp;
> > > } else {
> > > return _VSTD::move(__tmp).operator->();
> > > }
> > > }
> > > #else
> > > pointer operator->() const {
> > > return _VSTD::addressof(operator*());
> > > }
> > > #endif // _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_CONCEPTS)
> > > ```
> > > Drive-by removed the cute spacing in `pointer operator->`.
> > Done (with very slight modification).
> >
> > Do you think it's worth thinking whether it's possible to test for the change in the effects of this function in C++20?
> Yes, you certainly could write a test for it, so sure, let's do it!
> ```
> struct It {
> [...all the usual members...]
> int& operator*() const; // not defined
> int *operator->() const&; // not defined
> int *operator->() && { return nullptr; }
> };
> std::reverse_iterator<It> r;
> assert(r.operator->() == nullptr);
> ```
Done, thanks! (I've had to provide definitions for some of the unimportant functions -- otherwise I'm getting errors like
```
function 'main(int, char **)::It::operator*' has internal linkage but is not defined [-Werror,-Wundefined-internal]
```
).
================
Comment at: libcxx/include/__iterator/reverse_iterator.h:172-178
+ _LIBCPP_HIDE_FROM_ABI friend constexpr
+ iter_rvalue_reference_t<_Iter> iter_move(const reverse_iterator& __i)
+ noexcept(is_nothrow_copy_constructible_v<_Iter>
+ && noexcept(ranges::iter_move(--declval<_Iter&>()))) {
+ auto __tmp = __i.base();
+ return ranges::iter_move(--__tmp);
+ }
----------------
Quuxplusone wrote:
> var-const wrote:
> > Quuxplusone wrote:
> > > clang-format strikes again. Ditto `iter_swap`.
> > Nope, I simply prefer this formatting. Changed.
> FWIW, I have no strong preference myself on whether the arguments should be vertically aligned or (2|4)-space-indented. But I do have a strong preference that when breaking a binary operation, the operator/separator should always be attached to the line //above//, not the line //below//:
> ```
> int a = x +
> y; // good
> int b = x
> + y; // bad
> ```
> (with libc++'s style for `X() : a_()` being a notable exception, and `a ? b : c` ambivalent)
FWIW, I prefer the opposite -- I find it easier when for each subexpression, the operand is right in front of it so I don't have to scan the previous lines (of varying length) to find it. It also makes it easier to insert lines or move them around because this way, they are more "self-contained".
================
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:
> > 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--`).
================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/iterator_concept_conformance.compile.pass.cpp:64-65
+static_assert(!std::sized_sentinel_for<unsized_iterator, unsized_iterator>);
+static_assert(!std::sized_sentinel_for<std::reverse_iterator<unsized_iterator>,
+ std::reverse_iterator<unsized_iterator>>);
+
----------------
Quuxplusone wrote:
> If this line is too long, consider (1) renaming `unsized_iterator` to `It`, (2) moving the tests into a (never-called) `void test() {` purely so that you can use nice short `{ }` scopes for these typedef names.
Renamed to `{,un}sized_it` (but personally I think that trying to avoid line breaks isn't really worth it).
================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/iterator_concept_conformance.compile.pass.cpp:58-60
+using sized_iterator = random_access_iterator<int*>;
+static_assert( std::sized_sentinel_for<sized_iterator, sized_iterator>);
+static_assert( std::sized_sentinel_for<std::reverse_iterator<sized_iterator>, std::reverse_iterator<sized_iterator>>);
----------------
Quuxplusone wrote:
> var-const wrote:
> > Quuxplusone wrote:
> > > These three lines don't seem to add anything, but it would be nice to have a test for some type `B` such that `bidirectional_iterator<B> && !random_access_iterator<B> && sized_sentinel_for<B, B>`. Unfortunately I think we don't have such a type in test_iterators.h, but it might take only ~15 lines to write one here (especially since you don't have to //implement// any of its methods, just declare them).
> > >
> > > I also think you should actually split these lines out into `reverse.iterators/sized_sentinel.compile.pass.cpp`, and add another test for a type `R` such that `essentially-random_access_iterator<R> && disable_sized_sentinel_for<R, R>`, and make sure that `requires { reverse_iterator<R>() - reverse_iterator<R>(); } == false`.
> > > These three lines don't seem to add anything
> >
> > The main intention for these lines is to provide more context for the actual test rather than test anything on their own. Basically, "if the underlying iterators are sized, sized sentinel for `reverse_iterator` works; otherwise, it doesn't". They also verify that the reason `sized_sentinel_for` is false is because of the underlying iterators, not because it's written in such a way that it's always false.
> I'd still like you to create a `reverse.iterators/sized_sentinel.compile.pass.cpp` with the two tests I suggest in the parent comment. We need to test the interaction with `disable_sized_sentinel_for` and the effect on the `reverse_iterator`'s own `operator-`.
Moved the tests into a new file.
> add another test for a type `R` such that `essentially-random_access_iterator<R> && disable_sized_sentinel_for<R, R>`, and make sure that `requires { reverse_iterator<R>() - reverse_iterator<R>(); } == false`.
I'm probably missing something, but `reverse_iterator::operator-` doesn't check for `sized_sentinel_for` directly, instead relying on whether `operator-` is defined for the underlying iterators. It seems like this test would check the behavior of the underlying iterators.
================
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:
> 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.
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