[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:49-57
 template <std::input_or_output_iterator I>
 constexpr void check_assignable_case(std::ptrdiff_t const n) {
   auto range = range_t{0, 1, 2, 3, 4, 5, 6, 7, 8, 9};
-  auto result =
-      std::ranges::next(stride_counting_iterator(I(range.begin())), stride_counting_iterator(I(range.begin() + n)));
+  auto first = stride_counting_iterator(I(range.begin()));
+  auto last = stride_counting_iterator(I(range.begin() + n));
+  auto result = std::ranges::next(std::move(first), std::move(last));
   assert(result.base().base() == range.begin() + n);
----------------
Ooh, this test is super subtle. I recommend rewriting it as
```
template <class It>
constexpr void check_assignable_sentinel_case() {
  int range[] = {1, 2, 3, 4};
  It first = It(stride_counting_iterator<int*>(range + 1));
  It last = It(stride_counting_iterator<int*>(range + 4));
  It result = std::ranges::next(std::move(first), std::move(last));
  assert(base(result).base() == range + 4);  // i.e. last
  assert(result.stride_count() == 0);  // because we got here by copying last's value, not by incrementing
}
[...]
check_assignable_sentinel_case<cpp17_input_iterator<stride_counting_iterator<int*>>>();
check_assignable_sentinel_case<cpp20_input_iterator<stride_counting_iterator<int*>>>();
check_assignable_sentinel_case<output_iterator<stride_counting_iterator<int*>>>();
check_assignable_sentinel_case<forward_iterator<stride_counting_iterator<int*>>>();
```
etc. etc. In particular, I'm willing to believe sans tests that `cpp17_input_iterator<stride_counting_iterator<int*>>` is a cpp17_input_iterator, but I'm not willing to believe the same of `stride_counting_iterator<cpp17_input_iterator<int*>>`.


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


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator_sentinel.pass.cpp:87-91
   check_assignable_case<cpp17_input_iterator<range_t::const_iterator> >(1);
   check_assignable_case<forward_iterator<range_t::const_iterator> >(3);
   check_assignable_case<bidirectional_iterator<range_t::const_iterator> >(4);
   check_assignable_case<random_access_iterator<range_t::const_iterator> >(5);
   check_assignable_case<contiguous_iterator<range_t::const_iterator> >(6);
----------------
Please replace `cpp17_input_iterator<range_t::const_iterator>` with `cpp17_input_iterator<const int*>` and so on, throughout this file and anywhere else you see this pattern happening.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.prev/iterator.pass.cpp:23-26
   constexpr auto range = std::array{0, 1, 2, 3, 4, 5, 6, 7, 8, 9};
   assert(std::ranges::prev(bidirectional_iterator(&range[4])) == bidirectional_iterator(&range[3]));
   assert(std::ranges::prev(random_access_iterator(&range[5])) == random_access_iterator(&range[4]));
   assert(std::ranges::prev(contiguous_iterator(&range[6])) == contiguous_iterator(&range[5]));
----------------
FWIW, I'd prefer to see this simplified down to
```
template<class It>
constexpr bool test() {
  int range[] = {0, 1, 2, 3, 4, 5};
  assert(std::ranges::prev(It(range+1)) == It(range));
  assert(std::ranges::prev(It(range+5)) == It(range+4));
  assert(std::ranges::prev(It(range+6)) == It(range+5));
  return true;
}

int main(int, char**) {
  static_assert(test<bidirectional_iterator<int*>>());
  static_assert(test<random_access_iterator<int*>>());
  static_assert(test<contiguous_iterator<int*>>());
  static_assert(test<int*>());
  test<bidirectional_iterator<int*>>();
  test<random_access_iterator<int*>>();
  test<contiguous_iterator<int*>>();
  test<int*>();
}
```
This shortens the line lengths, and makes sure we hit the "one-past-the-end" case in constexpr for all kinds of iterators including raw pointers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103272