[libcxx-commits] [PATCH] D100073: [libcxx] adds `std::indirectly_readable` to <iterator>

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM with comments addressed. I marked blocking comments as such and non-blocking comments as such.



================
Comment at: libcxx/include/iterator:2516-2531
+// [iterator.concept.readable]
+template<class _In>
+concept __indirectly_readable_impl =
+  requires(const _In __in) {
+    typename iter_value_t<_In>;
+    typename iter_reference_t<_In>;
+    typename iter_rvalue_reference_t<_In>;
----------------
This LGTM.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.readable/indirectly_readable.compile.pass.cpp:120
+struct
+    no_common_reference_between_rvalue_ref_to_iter_ref_and_rvalue_ref_to_iter_rvalue_ref {
+  using value_type = iter_ref2;
----------------
You could use shorter names and instead use a comment to explain what this specific sub-test does. This way, you wouldn't have to carry around a name with almost 120 characters, which is kind of heavy on the eye and obscures the intent of the test.

This one is blocking.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/read_write.h:1
+//===----------------------------------------------------------------------===//
+//
----------------
Does it make sense to move this to `test/support` instead? Non blocking.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100073