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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver accepted this revision as: zoecarver.
zoecarver added a comment.

This still mostly LGTM sans a few small comments.

Do you want to just merge this and indirectly writable into one patch? I'd be fine with that FWIW and it might make it easier to keep track of on your end.

Also, for what it's worth: I don't have a preference as to how you test the standard types. I think the current implementation (have each test be its own file) is nice, but also looks like a lot of work to write. So in the future, do whatever you'd prefer/is easier (imho).



================
Comment at: libcxx/include/__iterator/iterator_traits.h:42
+};
+
 // [iterator.traits]
----------------
Shouldn't these go in `__iterator/concepts.h`?


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.readable/indirectly_readable.compile.pass.cpp:88
+template <>
+struct common_reference<iter_ref1&, iter_ref1&&> {};
+
----------------
Non-blocking, nit: Apparently we're suppose to do this without opening the std namespace. 


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.readable/indirectly_readable.compile.pass.cpp:104
+
+struct unrelated_iter_ref_rvalue_and_iter_rvalue_ref_rvalue {
+  using value_type = iter_ref2;
----------------
This name is long and hard to grok. Let's call it something like `unrelated_ref`.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.writable/indirectly_writable.compile.pass.cpp:46
+static_assert(!check_indirectly_writable<void const volatile*, int>());
+static_assert(!check_indirectly_writable<void*, double>());
+static_assert(check_indirectly_writable<void**, int*>());
----------------
Non-blocking: I'm not sure we need to check this and `void*, int`.Same below. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100073