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

Arthur O'Dwyer via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Mon Feb 28 07:51:58 PST 2022


Quuxplusone 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);
+    }
----------------
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.


================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/iter_move.pass.cpp:29
+
+template <class T>
+struct Iterator {
----------------
I don't think you ever use the fact that `adl::Iterator` is a template. Please remove the templateness.


================
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);
----------------
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.


================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/iter_move.pass.cpp:89-93
+
+    ++ri;
+    x = iter_move(ri);
+    assert(x == 1);
+    assert(adl::iter_move_invocations == 2);
----------------
I suggest removing lines 89-93. We don't need to test `operator++` here.


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


================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/iter_swap.pass.cpp:31
+
+template <class T>
+struct Iterator {
----------------
Ditto here: I don't think you use the fact that `adl::Iterator` is a template.


================
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 <>
----------------
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.


================
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&;
----------------
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??


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