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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

A few comments, nothing big. Overall LGTM, thanks.

This supersedes D102789 <https://reviews.llvm.org/D102789> (mentioning so it gets cross-referenced).



================
Comment at: libcxx/docs/Status/Cxx2bIssues.csv:101
 `2191 <https://wg21.link/LWG2191>`__,"Incorrect specification of ``match_results(match_results&&)``","October 2021","",""
 `2381 <https://wg21.link/LWG2381>`__,"Inconsistency in parsing floating point numbers","October 2021","",""
 `2762 <https://wg21.link/LWG2762>`__,"``unique_ptr operator*()`` should be ``noexcept``","October 2021","",""
----------------
Can you add an entry for not-yet-voted `LWG3664` and mark it as implemented?

> However, @tcanens pointed out a problem with the P/R: you can have an evil sentinel that is a `sized_sentinel_for<int*>` but doesn't support `last - first` when `first` is not literally an `int*`.

So do I understand correctly that the resolution of `LWG3664` might change? I just want to be careful to avoid implementing stale resolutions.


================
Comment at: libcxx/docs/Status/RangesPaper.csv:74
 | `ranges::next <https://llvm.org/D102563>`_
-| `ranges::prev <https://llvm.org/D102564>`_",[iterator.concepts],Christopher Di Bella,In progress
+| `ranges::prev <https://llvm.org/D102564>`_",[iterator.concepts],Christopher Di Bella,Complete
 [predef.iterators],Updates to predefined iterators.,"| [iterator.concepts]
----------------



================
Comment at: libcxx/docs/Status/RangesPaper.csv:74
 | `ranges::next <https://llvm.org/D102563>`_
-| `ranges::prev <https://llvm.org/D102564>`_",[iterator.concepts],Christopher Di Bella,In progress
+| `ranges::prev <https://llvm.org/D102564>`_",[iterator.concepts],Christopher Di Bella,Complete
 [predef.iterators],Updates to predefined iterators.,"| [iterator.concepts]
----------------
var-const wrote:
> Quuxplusone wrote:
> > philnik wrote:
> > > 
> > Meh, we only have one row here, and he did 3/4 of them.
> You can do `Christopher Di Bella and Arthur O'Dwyer` (see line 100).
I've used `various` in the past too. Just to clarify, the goal of these is not attribution, it's only to keep track of who's doing what. In fact, this page will be removed as soon as all of the original "One Ranges Proposal" is fully implemented, and then we'll fold that into a simple green check-mark in front of P0896.


================
Comment at: libcxx/include/__iterator/distance.h:81
+    } else {
+      return __last - decay_t<_Ip>(__first);
+    }
----------------
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`?


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.distance/iterator_sentinel.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
We seem to be missing tests that count the number of operations we're doing. Can you add some using `stride_counting_iterator`?


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

https://reviews.llvm.org/D117940