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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/include/iterator:477
+template<class _Ip>
+requires is_array_v<_Ip>
+struct indirectly_readable_traits<_Ip> {
----------------
Quuxplusone wrote:
> 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.)
It's not misformatted, so there's nothing to fix.


================
Comment at: libcxx/include/iterator:517
+struct indirectly_readable_traits<_Tp>
+  : __cond_value_type<typename _Tp::value_type> {};
+
----------------
Quuxplusone wrote:
> 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?
> there's probably some .csv file that needs to be updated to say that we implement the fix

Done.

> 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?

```
struct S {
  using value_type = int const volatile;
  using element_type = int const volatile;
};
```

It's left as an exercise for the reader to evaluate the type `std::indirectly_readable_traits<S>::type` by hand.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.pass.cpp:66
+  return result;
+}
+
----------------
Quuxplusone wrote:
> This function should use regular `assert` and `return true;` at the end.
See https://reviews.llvm.org/D99141#inline-935428.


================
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>);
----------------
Quuxplusone wrote:
> 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>`.
> 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.

I was under the impression that they weren't supported by `indirectly_readable_traits`, but it so happens that they are, and they're only blocked by `indirectly_readable`. Since I found that surprising, I decided that including them as tests would serve as good documentation.

> Check `vector<int>::{const_,}iterator`.
> If we're checking specific library types, I'd probably add `istream_iterator<int>`.

Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99461