[libcxx-commits] [PATCH] D101922: [libcxx][iterator] adds `std::ranges::advance`

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/advance.verify.cpp:39
+  advance(x, x); // expected-error {{use of undeclared identifier 'advance'}}
+  // expected-error@*:*{{no matching function for call to '__convert_to_integral'}}
+  advance(x, 0, x); // expected-error {{use of undeclared identifier 'advance'}}
----------------
Quuxplusone wrote:
> I don't think we can have tests in `test/std/` that depend on the internals of libc++ in this way.
> You could do this as a SFINAE test, like,
> ```
> template<class T>
> concept HasADLAdvance = requires(T x) { advance(x, 0); }
>     || requires(T x) { advance(x, x); }
>     || requires(T x) { advance(x, 0, x); };
> template<class T>
> concept IsAddressable = requires(T x) { &x; } || std::movable<T>;
> static_assert(!HasADLAdvance<std::ranges::dangling*>);
> static_assert(!IsAddressable<decltype(std::ranges::advance)>);
> ```
> (I also recommend using `std::ranges::dangling*` or something like that, because it's way lighter-weight than `std::ranges::forward_iterator` and also does not require you to reopen `namespace std` in this test file.)
> I don't think we can have tests in `test/std/` that depend on the internals of libc++ in this way.

Is this in regard to avoiding //just// `// expected-error@*:*{{no matching function for call to '__convert_to_integral'}}`? That's an instantiation error I didn't try too hard to fix, so I could revisit it.

> You could do this as a SFINAE test, like,
> ```
> template<class T>
> concept HasADLAdvance = requires(T x) { advance(x, 0); }
>     || requires(T x) { advance(x, x); }
>     || requires(T x) { advance(x, 0, x); };
> template<class T>
> concept IsAddressable = requires(T x) { &x; } || std::movable<T>;
> static_assert(!HasADLAdvance<std::ranges::dangling*>);
> static_assert(!IsAddressable<decltype(std::ranges::advance)>);
> ```

Why is `IsAddressable` a disjunction instead of a conjunction? (Also, why is movability involved at all?).

> (I also recommend using `std::ranges::dangling*` or something like that, because it's way lighter-weight than `std::ranges::forward_iterator` and also does not require you to reopen `namespace std` in this test file.)

`std::ranges::view_base` is already in, so I'll use that instead.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101922