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

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

Quuxplusone marked 4 inline comments as done.
Quuxplusone added inline comments.


================
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]
----------------
philnik wrote:
> 
Meh, we only have one row here, and he did 3/4 of them.


================
Comment at: libcxx/include/__iterator/distance.h:63
+struct __fn {
+  template<input_or_output_iterator _Ip, sentinel_for<_Ip> _Sp>
+    requires (!sized_sentinel_for<_Sp, _Ip>)
----------------
Mordante wrote:
> `input_or_output_iterator` doesn't match the description. Should this be  `input_or_output_iterator` if just `class _Ip`?
Oops, good catch. This should be `class _Ip`. (The constraint is superfluous, because it's already implied by `sentinel_for<_Ip> _Sp` right afterward. Therefore, we remove the superfluous constraint, to save the environment.)


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.distance/iterator_sentinel.pass.cpp:112-113
+    Sent last = Sent(It(a + 3));
+    static_assert(!std::is_invocable_v<decltype(std::ranges::distance), It&, Sent&>);
+    static_assert(!std::is_invocable_v<decltype(std::ranges::distance), It&, Sent&&>);
+    assert(std::ranges::distance(It(a), last) == 3);
----------------
philnik wrote:
> Please move the `static_assert`s together.
These four tests are always in the same order: lvalue-lvalue, lvalue-rvalue, rvalue-lvalue, rvalue-rvalue.
Although, in this case, I did throw in line 115, which broke up the pattern a bit. I should just remove line 115.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.distance/range.pass.cpp:52
+    assert(std::ranges::distance(r) == 3);
+    assert(std::ranges::distance(static_cast<R&&>(r)) == 3);
+    static_assert(!std::is_invocable_v<decltype(std::ranges::distance), const R&>);
----------------
philnik wrote:
> Why not `std::move`?
I was waffling between using `static_cast` for all of the cvref-qualifications, or using the more "normal-looking" versions; e.g. line 51 could have said `static_cast<R&>(r)` too. I think I'll change line 39 to //avoid// std::move, which might help avoid the question. :)


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

https://reviews.llvm.org/D117940