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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver requested changes to this revision.
zoecarver added a comment.
This revision now requires changes to proceed.

I have at least these few small comments, so please hold off on committing.

I'm not done reviewing yet, but I saw @ldionne approved it, so I wanted to get these in before you land it.



================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.readable/indirectly_readable.compile.pass.cpp:38
+
+#include "../read_write.h"
+
----------------
I suppose I don't feel strongly, but are you using this elsewhere? Otherwise, maybe it makes sense to line the header; it's only 30 lines. WDYT?


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.readable/indirectly_readable.compile.pass.cpp:57
+
+struct alternative_indirection {
+  using element_type = long;
----------------
Might as well put this in the header too, no? I think it makes sense to have all these types live together. 


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.readable/indirectly_readable.compile.pass.cpp:93
+
+struct missing_iter_value_t {
+  int operator*() const;
----------------
Nit: Maybe `missing_value_type` is more accurate? 


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.readable/indirectly_readable.compile.pass.cpp:98
+
+struct lvalue_ref_has_no_common_type_with_rvalue_ref {};
+
----------------
This type looks unused.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.readable/indirectly_readable.compile.pass.cpp:114
+};
+static_assert(!check_indirectly_readable<bad_iter_reference_t>());
+
----------------
What's the difference between this test and `indirection_mismatch`? Maybe `value_type` should be `iter_ref1` here? Because this should still fail, right? 


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.readable/indirectly_readable.compile.pass.cpp:128
+    !check_indirectly_readable<
+        no_common_reference_between_rvalue_ref_to_iter_ref_and_rvalue_ref_to_iter_rvalue_ref>());
+
----------------
This looks very similar to `iter_move_mismatch`. What's the difference? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100073