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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103272