[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 8 23:11:14 PST 2022
var-const added inline comments.
================
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&>()))) {
----------------
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?
================
Comment at: libcxx/include/__iterator/reverse_iterator.h:81
+ using reference = typename iterator_traits<_Iter>::reference;
#endif
----------------
Quuxplusone wrote:
> var-const wrote:
> > Quuxplusone wrote:
> > > Please group these blocks together; see what I did in https://reviews.llvm.org/D117656#change-18S54d7VLfJo
> > > I also recommend getting rid of the cute space-alignment formatting as long as you're in the area.
> > PTAL -- is this what you had in mind?
> Close. I'd definitely move `iterator_concept` down to coalesce those two `_LIBCPP_STD_VER > 17` blocks together. I'd also either duplicate the `iterator_category` and `pointer` lines, or move them up next to `iterator_type`, to keep the non-ifdef'ed stuff in a block together.
Done (moved up the common lines to avoid duplication).
================
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:
> > > 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]
> > ```
> > ).
> I finally notice your slight modification, and I'm not thrilled by it. Let's not break apart
> ```
> _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
> pointer operator->() const
> ```
> from the function's curly-braced body just to save two lines of duplication. We generally try (and everyone should generally try ;)) to put preprocessor conditionals only around "sensible" units of code — like, (an extreme example,) prefer
> ```
> #if X
> return x+1;
> #else
> return x;
> #endif
> ```
> over
> ```
> return x
> #if X
> +1
> #endif
> ;
> ```
Ok, I reverted to your original version with another slight modification -- duplicated `_LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14` between the two clauses as well (which seems to be consistent with your comment -- I think you originally omitted it for brevity, right?).
================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/types.pass.cpp:85-94
+namespace std {
+template <>
+struct iterator_traits<BarIter> {
+ using difference_type = char;
+ using value_type = char;
+ using pointer = char*;
+ using reference = char&;
----------------
Quuxplusone wrote:
> var-const wrote:
> > Quuxplusone wrote:
> > > Is the purpose of this test just to make sure that `reverse_iterator<BarIter>` takes its `difference_type` from `iterator_traits<BarIter>::difference_type` and not from `BarIter::difference_type` directly? ...But wait, `BarIter::difference_type` doesn't exist! And it wouldn't exist for an iterator type like `int*`, either. So I actually don't think this test is adding any coverage, and therefore would prefer to remove it. Unless I'm missing something.
> > >
> > > Which I must be, because how on earth do we end up with `is_same_v<typename std::reverse_iterator<BarIter>::reference, bool&>` on line 112??
> > > Is the purpose of this test just to make sure that `reverse_iterator<BarIter>` takes its `difference_type` from `iterator_traits<BarIter>::difference_type`..?
> >
> > `BarIter` is only for testing `reference` (to make sure it's implemented correctly both pre- and post-C++20). `difference_type` is tested via `FooIter`.
> >
> > > how on earth do we end up with `is_same_v<typename std::reverse_iterator<BarIter>::reference, bool&>` on line 112?
> >
> > Pre-C++20, `reference` would be defined as `iterator_traits<I>::reference`, so it's taken from the specialization of `iterator_traits`. In C++20+, `reference` is `iter_reference_t`, which is defined as `decltype(*declval<T&>());`. So the type is taken from the return type of `BarIter::operator*()` (see line `82`).
> Btw, I believe my confusion at first was due mainly to thinking `FooIter` and `BarIter` were the same type (i.e. I misread `Bar` as `Foo` or vice versa). I see where `bool` comes from, now.
>
> It strikes me that `FooIter` is just plain old bidirectional_iterator — er, kinda sorta — whenever it is //not// true that `TEST_STD_VER > 17`. So really, `FooIter` is a "C++20-only test case"; you dutifully assert things about it when `<= 17`, but those things aren't really interesting things, because half of the type is missing. I think the entire definition of `FooIter` should be guarded by `TEST_STD_VER > 17`.
>
> As I type that comment (specifically the "kinda sorta" part), I recall why inheriting from test iterators is a bad idea: you'll find that `std::bidirectional_iterator<FooIter> == false`, am I right? In fact `FooIter` isn't any kind of iterator in C++20, right? This doesn't render the test UB, but it does mean that you're not actually saving too much boilerplate here — you can //omit// all the iterator boilerplate! I bet you don't need much more than
> ```
> #if TEST_STD_VER > 17
> struct FooIter {
> using value_type = void*;
> using difference_type = void*;
> };
> template <>
> struct std::indirectly_readable_traits<FooIter> {
> using value_type = int;
> };
> template <>
> struct std::incrementable_traits<FooIter> {
> using difference_type = char;
> };
>
> static_assert(std::is_same_v<typename std::reverse_iterator<FooIter>::value_type, int>);
> static_assert(std::is_same_v<typename std::reverse_iterator<FooIter>::difference_type, char>);
> #endif // TEST_STD_VER > 17
> ```
>
> The `BarIter` test should remain enabled for all C++ versions, i.e. something like
> ```
> struct BarIter {
> bool& operator*() const;
> };
> template <>
> struct std::iterator_traits<BarIter> {
> using difference_type = char;
> using value_type = char;
> using pointer = char*;
> using reference = char&;
> using iterator_category = std::bidirectional_iterator_tag;
> };
>
> #if TEST_STD_VER > 17
> static_assert(std::is_same_v<typename std::reverse_iterator<BarIter>::reference, bool&>);
> #else
> static_assert(std::is_same_v<typename std::reverse_iterator<BarIter>::reference, char&>);
> #endif
> ```
Done.
I still need to define the classic 5 types because `reverse_iterator` inherits from `iterator_traits` (evidently `_LIBCPP_ABI_NO_ITERATOR_BASES` is false when running tests, in my environment at least). And `operator*` for good measure (because of how `reference` is defined).
I think the issue with inheritance that you mention can be solved via CRTP.
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