[libcxx-commits] [PATCH] D101922: [libcxx][iterator] adds `std::ranges::advance`

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added a comment.

I may have a few more comments later.



================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/advance.pass.cpp:33
+constexpr void check_round_trip(stride_counting_iterator<I> const& i, std::ptrdiff_t const n) {
+  auto const distance = n < 0 ? -n : n;
+  assert(i.distance_traversed() == distance);
----------------
Nit: I think `abs` would make this clearer. 


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/advance.pass.cpp:40
+constexpr void check_round_trip(stride_counting_iterator<I> const& i, std::ptrdiff_t const n) {
+  assert(i.distance_traversed() <= 1);
+  assert(i.displacement() == n < 0 ? -1 : 1);
----------------
When would this not be exactly one?


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/advance.pass.cpp:40
+constexpr void check_round_trip(stride_counting_iterator<I> const& i, std::ptrdiff_t const n) {
+  assert(i.distance_traversed() <= 1);
+  assert(i.displacement() == n < 0 ? -1 : 1);
----------------
zoecarver wrote:
> When would this not be exactly one?
Maybe `i.distance_traversed() == 1 || n & i.distance_traversed() == 0`.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/advance.pass.cpp:77
+  // Zero should be checked for each case and each overload
+  check_move_forward<cpp17_input_iterator<range_t::const_iterator> >(0);
+  check_move_forward<random_access_iterator<range_t::const_iterator> >(0);
----------------
Might as well test them all with N == 0.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/advance.pass.cpp:112
+template <std::input_or_output_iterator I>
+class sentinel_wrapper {
+public:
----------------
I feel like there've been a few times where I could have used a type like this. Maybe move it into the `test_iterators.h` header?


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/advance.pass.cpp:127
+template <std::input_or_output_iterator I, std::sentinel_for<I> S = 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};
----------------
What is the "assignable case"?


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/advance.pass.cpp:155
+[[nodiscard]] constexpr bool test() {
+  check_assignable_case<cpp17_input_iterator<range_t::const_iterator> >(1);
+  check_assignable_case<forward_iterator<range_t::const_iterator> >(3);
----------------
Why is `2` missing? If you're going to have seven, might as well have them all. 

Edit: I see, maybe make a comment about why we can't use cpp20_iterator. 


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/advance.pass.cpp:162
+
+  check_sized_sentinel_case<cpp17_input_iterator<range_t::const_iterator> >(1);
+  check_sized_sentinel_case<cpp20_input_iterator<range_t::const_iterator> >(2);
----------------
This suggestion might not actually be helpful, so feel free to just ignore it. But I was thinking, maybe we don't always want to test input iterator with N == 1, so maybe make this go 7, 6, 5, ...


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/advance.pass.cpp:191
+  auto const result = std::ranges::advance(current, n, distance_apriori_sentinel(range.size()));
+  assert(current.base().base() == expected.coordinate);
+  assert(result == expected.result);
----------------
If you dereference this, it should always be `n`, right? Maybe that's simpler and then we can get rid of `expected_t`. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101922