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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103272