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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/include/__iterator/primitives.h:60-70
+      while (__n > 0) {
+        --__n;
+        ++__i;
+      }
+
+      // Otherwise, decrements `i` by `-n`.
+      if constexpr (bidirectional_iterator<_Ip>) {
----------------
cjdb wrote:
> ldionne wrote:
> > I understand why this works, but I find it's a slightly confusing way to write this. Instead, Maybe `else if constexpr (is_bidirectional_iterator<_Ip>) { handle bidirectional case } else { handle-forward-only-case }`.
> > 
> > It's not as clever, but maybe easier to understand?
> I can do that, but it results in code duplication.
> 
> ```
> else if constexpr (bidirectional_iterator<_Ip>) {
>   while (__n > 0) {
>     --__n;
>     ++_i;
>   }
>   while (__n < 0) {
>     ++__n;
>     --__i;
>   }
> }
> else {
>   while (__n > 0) {
>     --__n;
>     ++_i;
>   }
> }
> ```
> I suppose I could put the forward case into an `__advance_forward` and call that twice. WDYT?
> ```
> else if constexpr (bidirectional_iterator<_Ip>) {
>   __advance_forward(__i, __n);
>   __advance_backward(__i, __n);
> }
> else {
>   __advance_forward(__i, __n);
> }
> ```
PTAL.


================
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);
----------------
zoecarver wrote:
> Nit: I think `abs` would make this clearer. 
`abs` is not `constexpr`.


================
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:
> zoecarver wrote:
> > When would this not be exactly one?
> Maybe `i.distance_traversed() == 1 || n & i.distance_traversed() == 0`.
In the case where assignment happens, which has `distance_traversed == 0`.


================
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};
----------------
zoecarver wrote:
> What is the "assignable case"?
http://eel.is/c++draft/range.iter.ops#range.iter.op.advance-4.1


================
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);
----------------
zoecarver wrote:
> If you dereference this, it should always be `n`, right? Maybe that's simpler and then we can get rid of `expected_t`. 
The point of these tests is to confirm that?


================
Comment at: libcxx/test/support/test_iterators.h:727
+
+  [[nodiscard]] constexpr I base() const& requires std::copyable<I> { return base_; }
+
----------------
ldionne wrote:
> `s/[[nodiscard]]//g`
I've removed it for some areas, but kept it where I think calling and not using is error-prone.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101922