[libcxx-commits] [PATCH] D103272: [libc++] Refactor the ranges::prev and ranges::next tests

Fri Jun 11 15:14:07 PDT 2021

Quuxplusone added inline comments.

================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator.pass.cpp:22-28
+  int range[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9};
+  assert(&*std::ranges::next(It(&range[0])) == &range[1]);
+  assert(&*std::ranges::next(It(&range[1])) == &range[2]);
+  assert(&*std::ranges::next(It(&range[2])) == &range[3]);
+  assert(&*std::ranges::next(It(&range[3])) == &range[4]);
+  assert(&*std::ranges::next(It(&range[4])) == &range[5]);
+  assert(&*std::ranges::next(It(&range[5])) == &range[6]);
----------------
Simply:
```
int range[4] = {};
assert(base(std::ranges::next(It(range))) == range+1);
assert(base(std::ranges::next(It(range+3))) == range+4);
```

================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator.pass.cpp:43-44
 int main(int, char**) {
-  static_assert(check_iterator());
-  check_iterator();
+  static_assert(test());
+  test();
   return 0;
----------------
Bikeshed: Traditionally we put the runtime test first and the longer static_assert second.

================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator_sentinel.pass.cpp:52-53
+  {
+    auto result = std::ranges::next(std::move(it), std::move(last));
+    assert(&*result == expected);
+  }
----------------
Here and throughout this function, the `auto`s are really confusing. Is this `auto` the same thing as `I`?
Is `&*result` the same thing as `base(result)`? (`base` being an overload set defined in "test_iterators.h".)

================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator_sentinel.pass.cpp:120
+  check_assignable(random_access_iterator(&range[0]), random_access_iterator(&range[5]), &range[5]);
+  check_assignable(contiguous_iterator(&range[0]), contiguous_iterator(&range[6]), &range[6]);
+
----------------
Since you're changing this code anyway, FWIW I'd prefer
`forward_iterator<int*>(range+k)`
rather than
`forward_iterator(&range[k])`
throughout. (And I doubt that the variation of `k` — `0`, `1`, ... `7` — is pulling its weight. I'd leave them all as `FooIter<int*>(range)` for brevity, or else `FooIter<int*>(range+10)` for verification-that-these-pointers-are-never-dereferenced. And there's no reason `range` has to be so big; making it, say, size-4 instead of size-10 would be fine.)

================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator_sentinel.pass.cpp:81
   check_assignable_case<contiguous_iterator<range_t::const_iterator> >(6);
-  check_assignable_case<output_iterator<range_t::iterator> >(7);

----------------
ldionne wrote:
> Quuxplusone wrote:
> > ldionne wrote:
> > > Quuxplusone wrote:
> > > > zoecarver wrote:
> > > > > Do we want to add other checks for output_iterator? Why did you remove this check?
> > > > +1; I don't understand the bit of @ldionne's summary about not being able to use `next` on straight output iterators. cppreference thinks you can increment an output iterator just fine. So why was this line removed? And why weren't similar lines in other tests removed? (Grep the right side for `output_iterator` — we're still using it in plenty of places, e.g. line 32 of range.iter.ops.next/iterator.pass.cpp.) So I don't get it.
> > > Those overloads of `std::ranges::next` can't be used with an `output_iterator` out of the box, since `output_iterator` is missing a comparison operator (we need to compare against the sentinel). I'm still testing against `output_iterator`s for the `next(iter)` and `next(iter, n)` overloads, though.
> > Our comments crossed in the ether (and also my understanding of this test is evolving). `check_assignable_case` is super subtle: it does not depend on comparing iterators at all. It simply says, "Ah, `output_iterator` is assignable from `output_iterator`, so `std::ranges::next(it1, it2)` is simply the result of `it1 = it2`, full stop." There is no comparison involved.
> > See the sample implementation of `advance` here: https://en.cppreference.com/w/cpp/iterator/ranges/advance , the part that mentions `std::assignable_from`. (The sample implementation of `next` is done in terms of `advance`.)
> This is the signature of `next(it, sent)`:
> 
> ```
> template<input_or_output_iterator I, sentinel_for<I> S>
> constexpr I ranges::next(I x, S bound);
> ```
> 
> In particular, it requires `sentinel_for<I> S`, which is not satisfied here because `output_iterator<T>` is not a sentinel for `output_iterator<T>` (it's missing the comparison). So it's simply invalid to try and call `next(it, sent)` when both `it` and `sent` are an output iterator archetype - we'd have to add something to the archetype to make it model `sentinel_for`.
> 
> It is true the implementation doesn't technically require comparing against the sentinel, but the signature still requires it.
OK, I buy that. I think there should be a runtime test verifying that (although `operator==`/`!=` is required to exist) nothing except `operator=` assignment is ever actually called in this case.

================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.prev/constraints.compile.pass.cpp:23
+template <class ...Args>
+concept has_ranges_prev = requires (Args ...args) {
+  { std::ranges::prev(std::forward<Args>(args)...) };
----------------
I think you intended `Args&&... args` here. Otherwise the `forward` below should just be `move` (and it's certainly redundant either way, given that `Args...` here are always scalar types).

================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.prev/constraints.compile.pass.cpp:32-33
+
+// Test the test
+static_assert(has_ranges_prev<int*, std::ptrdiff_t, int*>);
----------------
I'd do this by
```
using Jt = bidirectional_iterator<int*>;
static_assert(has_ranges_prev<Jt>);
static_assert(has_ranges_prev<Jt, std::ptrdiff_t>);
static_assert(has_ranges_prev<Jt, std::ptrdiff_t, Jt>);
```

================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.prev/iterator.pass.cpp:22-25
+  int range[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9};
+  assert(std::ranges::prev(It(&range[4])) == It(&range[3]));
+  assert(std::ranges::prev(It(&range[5])) == It(&range[4]));
+  assert(std::ranges::prev(It(&range[6])) == It(&range[5]));
----------------
```
int range[4] = {};
assert(std::ranges::prev(It(range+1)) == It(range));
assert(std::ranges::prev(It(range+4)) == It(range+3));
```
It turns out I'm never happy. ;)

================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.prev/iterator_count.pass.cpp:30
+  } else {
+    auto const distance = n < 0 ? -n : n;
+    assert(result.stride_count() == distance);
----------------
`s/auto const/std::ptrdiff_t/` for sanity's sake.

================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.prev/iterator_count_sentinel.pass.cpp:31
+  {
+    stride_counting_iterator strided_it(it), strided_last(last);
+    auto result = std::ranges::prev(strided_it, n, strided_last);
----------------
Please declare one variable per line.
This smells like CTAD — is `stride_counting_iterator` a template? if so, could we write out its arguments?

================
Comment at: libcxx/test/support/test_iterators.h:732-734
+  [[nodiscard]] constexpr I const& base() const& { return base_; }

   [[nodiscard]] constexpr I base() && { return std::move(base_); }
----------------
While you're here, you might remove the [[nodiscard]].

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103272