[libcxx-commits] [PATCH] D102789: [libcxx][iterator] adds `std::ranges::distance`

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__iterator/distance.h:35
+  template <input_or_output_iterator _Ip, sentinel_for<_Ip> _Sp>
+  [[nodiscard]] constexpr iter_difference_t<_Ip> operator()(_Ip __first, _Sp __last) const {
+    if constexpr (sized_sentinel_for<_Sp, _Ip>) {
----------------
Drop `[[nodiscard]]`


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.distance/distance.pass.cpp:13
 
-// std::forward_iterator;
+// std::ranges::distance
 
----------------
Please split into two test files, one for each overload.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.distance/distance.pass.cpp:45
+constexpr void check_iterator_sentinel() {
+  assert(std::ranges::distance(I(range.begin()), sentinel_wrapper(range.begin() + 5)) == 5);
+  assert(std::ranges::distance(I(range.begin()), sentinel_wrapper(range.begin())) == 0);
----------------
Can we avoid using a global `range` here (and below) and instead have a local variable? It's a tiny difference but it helps local reasoning.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.distance/distance.verify.cpp:17
+
+namespace std::ranges {
+class forward_iterator {
----------------
Mordante wrote:
> cjdb wrote:
> > Quuxplusone wrote:
> > > I don't think you're (that is, I don't think the programmer is) allowed to add your own types to `namespace std::ranges`.
> > I'm aware users aren't allowed to do this, but we're testing the standard library here. We need to ensure that ADL doesn't find something in the same namespace as the function, but there's nothing to test with (no range adaptors have been merged with main yet).
> Would it make sense to add a TODO and improve this test after we have adaptors?
@cjdb Please add a TODO like you've done in other patches, we'll go back and fix it as soon as we check-in an adaptor.


================
Comment at: libcxx/test/support/test_iterators.h:877
+
+  [[nodiscard]] constexpr I const& base() const& requires std::copyable<I> { return base_; }
+
----------------
Drop `[[nodiscard]]` from the tests (here and elsewhere), except for really vexing cases as we've discussed (but I don't think there's any).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102789