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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/include/__iterator/distance.h:48-55
+  template <range _Rp>
+  [[nodiscard]] constexpr range_difference_t<_Rp> operator()(_Rp&& __r) const {
+    if constexpr (sized_range<_Rp>) {
+      return static_cast<range_difference_t<_Rp> >(ranges::size(__r));
+    } else {
+      return (*this)(ranges::begin(__r), ranges::end(__r));
+    }
----------------
Quuxplusone wrote:
> I notice we're not doing the `noexcept` thing here. Is that because https://eel.is/c++draft/range.iter.op.distance#3 says merely "equivalent to" instead of the term of power "expression-equivalent to"?
Yes. As it is, the noexcept specifiers would either be really difficult to read or would need their own special functions, neither of which I'm keen to do.

I've just noticed that having four overloads won't hurt readability like it did with `ranges::advance`, so I think I can get away with making `ranges::distance` noexcept without it being a colossal eyesore. (I might have fixed the problem with `ranges::advance`, so I'll need to go back and check soon.)


================
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 {
----------------
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).


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.distance/distance.verify.cpp:39-46
+// The function templates defined in [range.iter.ops] are not found by argument-dependent name lookup ([basic.lookup.argdep]).
+void no_adl_participation() {
+  std::ranges::forward_iterator i;
+  distance(i, i); // expected-error {{use of undeclared identifier 'distance'}}
+
+  std::ranges::some_range r;
+  distance(r); // expected-error {{use of undeclared identifier 'distance'}}
----------------
Quuxplusone wrote:
> We could do this in a .pass.cpp by SFINAE-testing the well-formedness of `distance(r)`, instead of relying on the exact wording of the compiler error and then having to skip the test for non-Clang compilers.
Something like this?
```
template<class T>
constexpr bool is_adl_friendly = requires(T t) { distance(t); };

static_assert(!is_adl_friendly<std::ranges::forward_iterator>);
static_assert(!is_adl_friendly<std::ranges::some_range>);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102789