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

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

Quuxplusone 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;
+
----------------
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.


================
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>'}}
+
----------------
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.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/incrementable.traits/iter_difference_t.pass.cpp:23-44
+  constexpr bool result = std::same_as<std::iter_difference_t<T>, Expected>;
+  static_assert(std::same_as<std::iter_difference_t<T const>, Expected> ==
+                result);
+  static_assert(std::same_as<std::iter_difference_t<T volatile>, Expected> ==
+                result);
+  static_assert(
+      std::same_as<std::iter_difference_t<T const volatile>, Expected> ==
----------------
FWIW, this indentation is insane.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99863