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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
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:
> > > 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".
> > easier to insert lines or move them around
> 
> It occurs to me that I'd definitely break `std::cout << "x" << "y"` between `"x"` and `<< "y"` (although in that case I'd also insert a new `; std::cout` so that the statement didn't need to span lines in the first place).
> For other operations like `x && y` or `x + y`, which don't tend to be chained extensively or ever have new operands inserted in the middle, [insert what I said before].
> Btw, I'm a big fan of "trailing comma" in enum definitions and array initializers, and I wish a trailing comma were syntactically allowed everywhere (e.g. function parameter lists, member-initializer-lists).
> ```
> enum E {
>     A,
>     B,
> };
> ``` 
> There we get the best of both worlds IMO: the comma gets to stay at the end of the line //and// we can easily insert new lines in the middle or at the end.
(I made the change, by the way)

I see your point, but I still find it easier to have a single rule that covers everything.


================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/iter_move.pass.cpp:81-84
+    constexpr int N = 3;
+    int a[N] = {0, 1, 2};
+
+    adl::Iterator<int> i(a + N);
----------------
Quuxplusone wrote:
> No big deal; but I prefer the shorter code with fewer identifiers on my mental "stack". Otherwise I'm looking and waiting for the place where `N` will be used, and it never comes.
> 
> Ditto throughout.
Unless you feel strongly, I'd rather keep as is. For me, the constant a) underscores that the exact value is unimportant; b) makes it obvious that this is the length of the array, as opposed to some arbitrary index within the array; c) prevents me from having to keep track whether the number is the same throughout the scope (with a constant, there would be a compiler error if it's mistyped).


================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/iter_move.pass.cpp:137
+      static_assert( std::is_nothrow_copy_constructible_v<NoexceptCopyThrowingMove>);
+      ASSERT_NOT_NOEXCEPT(std::ranges::iter_move(--std::declval<NoexceptCopyThrowingMove&>()));
+      using RI = std::reverse_iterator<NoexceptCopyThrowingMove>;
----------------
Quuxplusone wrote:
> What's the `--` doing here and on lines 115 and 159? You can remove it, right?
> ...er, actually, I guess the long names confused me. This line isn't testing anything about `reverse_iterator`, but merely about the underlying iterator type `NoexceptCopyThrowingMove` (which I would just call `It` in each case).
> So lines 136-137 are just testing the setup, and then line 139 is the important line.
> Personally I'd still remove lines 136-137, because line 139 is clear enough: we're testing that `iter_move` is noexcept iff its dependencies are noexcept.
> 
> IOW, I'd make this
> ```
>     {
>       struct It {
>         using value_type = int;
>         using difference_type = ptrdiff_t;
>         explicit It() noexcept;
>         It(const It&) noexcept;
>         int& operator*() const;  // if you need to define a body here, OK
>         It& operator++() noexcept;
>         It operator++(int) noexcept;
>         It& operator--() noexcept;
>         It operator--(int) noexcept;
>       };
>       std::reverse_iterator<It> ri;
>       ASSERT_NOT_NOEXCEPT(iter_move(ri));  // because operator*
>     }
> ```
> Oh hey, in replacing the name `NoexceptCopyThrowingMove` with `It`, I discovered that the old name was wrong! Type `It` has no move-ctor at all, much less a throwing one. The non-noexcept operations on lines 121-139 were `operator*` and `operator--` (I assume one or the other was accidental). So the type could have been named `NoexceptCopyThrowingDeref` or something like that... but fortunately now I don't have to come up with a name for it, because `It` is fine and good. :)
> What's the `--` doing here and on lines 115 and 159? 

I'm emulating how the `noexcept` specification is defined in `iter_move`:
```
noexcept(is_nothrow_copy_constructible_v<_Iter> &&
    noexcept(ranges::iter_move(--declval<_Iter&>()))
)
```

> I discovered that the old name was wrong! Type It has no move-ctor at all, much less a throwing one. 

You're right, thanks a lot for noticing this -- this is a mistake on my part. The name should replace `Move` with `Decrement`.

I think, however, that this is evidence in favor of keeping both the verbose (but descriptive) names and verifying the properties of the dependencies. Together, they add a certain redundancy (in the information-theory sense) that makes it easier to recover from mistakes. Imagine, for the sake of the argument, that it was the class implementation that was wrong (possibly due to copy-pasting), not the name. It would be much more difficult to notice with a name like `It`. When the name, the implementation and checking the properties reinforce each other, it's easier to see what exactly is being tested and whether it works like expected.


================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/types.pass.cpp:69-79
 #if TEST_STD_VER > 17
-    test<contiguous_iterator<char*>>();
-    static_assert(std::is_same_v<typename std::reverse_iterator<bidirectional_iterator<char*>>::iterator_concept, std::bidirectional_iterator_tag>);
-    static_assert(std::is_same_v<typename std::reverse_iterator<random_access_iterator<char*>>::iterator_concept, std::random_access_iterator_tag>);
-    static_assert(std::is_same_v<typename std::reverse_iterator<contiguous_iterator<char*>>::iterator_concept, std::random_access_iterator_tag>);
-    static_assert(std::is_same_v<typename std::reverse_iterator<char*>::iterator_concept, std::random_access_iterator_tag>);
+template <>
+struct std::indirectly_readable_traits<FooIter> {
+  using value_type = int;
+};
+
+template <>
----------------
Quuxplusone wrote:
> You know I'm never thrilled by tests that customize bits of types they don't own. Consider defining `struct FooIter` as its own type.
I understand your concern, but I strongly dislike increasing the amount of boilerplate. I would agree if this were a more "arbitrary" type, but our test iterators have well-defined semantics and quite a few dependencies, so it seems reasonable to rely on this class being there and not changing radically. So I'd rather keep as is, unless you feel strongly about it.


================
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:
> 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`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120180