[libcxx-commits] [PATCH] D99863: [libcxx] adds `iter_difference_t` and `iter_value_t`

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb marked 2 inline comments as done.
cjdb added inline comments.


================
Comment at: libcxx/include/iterator:549-569
+// Let `R[I]` be `remove_­cvref_­t<I>`. The type `iter_­value_­t<I>` denotes
+template<class>
+struct __extract_iter_value_t;
+
+template<class _Ip>
+using iter_value_t = typename __extract_iter_value_t<remove_cvref_t<_Ip>>::type;
+
----------------
Quuxplusone wrote:
> Slightly disentangled: so that code doesn't interrupt the flow of an English sentence, and the primary template isn't forward-declared.
> 
> I think this is fine; but it does feel weird at a higher level that http://eel.is/c++draft/sequences#general-2 already defines `__iter_value_type` (as an exposition-only implementation detail related to container deduction guides), and here http://eel.is/c++draft/readable.traits#2 is introducing `iter_value_t` with a subtly different meaning. libc++ maintainers will have to be careful to pick the right one.
The mixing of standardese with code was intentional, to show which bits of code correspond to which bits of the standard. This idea was taken from `<type_traits>`.

In future, for something as trivial as this, please request what you're after instead of providing a full-blown suggested edit.

> I think this is fine; but it does feel weird at a higher level that http://eel.is/c++draft/sequences#general-2 already defines `__iter_value_type` (as an exposition-only implementation detail related to container deduction guides), and here http://eel.is/c++draft/readable.traits#2 is introducing `iter_value_t` with a subtly different meaning. libc++ maintainers will have to be careful to pick the right one.

I think the best thing to do would be to ask LWG to consider renaming //`iter-value-type`// to something else (it might even be possible to do this editorially after a mailing discussion), or us just doing so regardless of what LWG thinks, so as to avoid confusion.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/incrementable.traits/iter_difference_t.fail.cpp:32
+using fails4 = std::iter_difference_t<void_subtraction>;
+// expected-error at iterator:* {{no type named 'difference_type' in 'std::iterator_traits<void_subtraction>'}}
+
----------------
Quuxplusone wrote:
> FWIW, I think it would make more sense to cast all these tests in terms of what we //do// expect to be true, instead of relying on compiler error messages.
> For example, if we expect that the type `std::iter_difference_t<void_subtraction>` doesn't exist, we could maybe do
> 
> ```
> template<class T> auto has_no_difference_type(int) -> std::void_t<std::iter_difference_t<T>> {}
> template<class T> bool has_no_difference_type(long) { return true; }
> 
> int main(int, char**)
> {
>     assert(has_no_difference_type<void_subtraction>(42));
> 
>     return 0;
> }
> ```
> But this is assuming that `iter_difference_t<T>` is supposed to be SFINAE-friendly. I don't know if that's actually guaranteed/intended by the Standard.
The wording is SFINAE-friendly, but my implementation here wasn't due to a mistake in translating from cjdb-ranges to libc++.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99863