[libcxx-commits] [PATCH] D117940: [libc++] [ranges] Implement std::ranges::distance

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__iterator/distance.h:81
+    } else {
+      return __last - decay_t<_Ip>(__first);
+    }
----------------
ldionne wrote:
> This is to handle the array case, right? I assume the answer is yes, but do you have a test case for this `if constexpr`?
The current (but problematic) P/R for LWG3664 fixes the array-vs-pointer case. This `if constexpr` is specifically for the array-vs-`EvilSentinel` case, and yes, that's tested.

A more helpful way to look at is probably: line 81 is actually the //happy// path, that works for almost all iterator types //and// for all array types... but line 81 //doesn't// work for move-only iterator types such as `cpp20_input_iterator`. For those move-only types, we must use line 78 instead.

This is my current-informally-suggested P/R for LWG3664. The P/R //will// change at some point, because we //need// `ranges::distance(a, a)` to work — it's just unacceptable for it not to work. Therefore, I'm willing to add a line for LWG3664 but wouldn't mark it "Complete", which also means maybe we don't need to add a line at all, and can just wait for LWG3664 to be adopted before we revisit and audit it. WDYT?


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.distance/iterator_sentinel.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
ldionne wrote:
> We seem to be missing tests that count the number of operations we're doing. Can you add some using `stride_counting_iterator`?
Sure, will do, although you probably know I'm not a fan of `stride_counting_iterator` in its current state.


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

https://reviews.llvm.org/D117940