[libcxx-commits] [PATCH] D99461: [libcxx] adds `std::indirectly_readable_traits` to <iterator>

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/iterator:477
+template<class _Ip>
+requires is_array_v<_Ip>
+struct indirectly_readable_traits<_Ip> {
----------------
Indent line 477 one level (two spaces). Ditto line 488.
(Please no objections along the lines of "but clang-format did it this way." We shouldn't check in misformatted code just because a machine produced it.)


================
Comment at: libcxx/include/iterator:513
+  same_as<remove_cv_t<typename _Tp::element_type>,
+          remove_cv_t<typename _Tp::value_type>>;
+
----------------
Make sure you have a test that fails if either of these `remove_cv_t` are missing. I suggest
```
struct X1 { using value_type = int; using element_type = const volatile int; };
struct X2 { using value_type = const volatile int; using element_type = int; };
static_assert(std::is_same_v<indirectly_readable_traits<X1>::value_type, int>);
static_assert(std::is_same_v<indirectly_readable_traits<X2>::value_type, int>);
```


================
Comment at: libcxx/include/iterator:517
+struct indirectly_readable_traits<_Tp>
+  : __cond_value_type<typename _Tp::value_type> {};
+
----------------
This implements https://cplusplus.github.io/LWG/issue3446 — excellent! — but that means there's probably some .csv file that needs to be updated to say that we implement the fix.

Er, actually, line 517 should say
```
  : __cond_value_type<remove_cv_t<typename _Tp::value_type>> {};
```
according to https://en.cppreference.com/w/cpp/iterator/indirectly_readable_traits , which implies that LWG3446 isn't telling the whole story. What's the deal here?


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.pass.cpp:28
+template <class T, class Expected>
+concept check_value_type_matches = check_has_value_type<T>&& std::same_as<
+    typename std::indirectly_readable_traits<T>::value_type, Expected>;
----------------
`> &&`


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.pass.cpp:66
+  return result;
+}
+
----------------
This function should use regular `assert` and `return true;` at the end.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.pass.cpp:107
+static_assert(check_value_type_matches<S2, int>);
+static_assert(check_value_type_matches<std::vector<int>, int>);
+static_assert(check_value_type_matches<std::optional<int>, int>);
----------------
AIUI, the point of `indirectly_readable` is that //`std::vector<int>::iterator`// should pass this test, and it's just an accident that `std::vector` and `std::optional` themselves pass it.
Check `vector<int>::{const_,}iterator`. Checking `vector` and `optional` seems fine but also unnecessary.
If we're checking specific library types, I'd probably add `istream_iterator<int>`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99461