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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__iterator/distance.h:81
+    } else {
+      return __last - decay_t<_Ip>(__first);
+    }
----------------
Quuxplusone wrote:
> 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?
Hmm, tricky. Let's not mention LWG3664 at all then, and we can audit the state of our implementation when it is actually filed.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.distance/iterator_sentinel.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Quuxplusone wrote:
> 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.
Yeah, me neither, but it still provides something of value for test coverage.


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

https://reviews.llvm.org/D117940