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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb marked 4 inline comments as done.
cjdb added a comment.

In D100073#2702490 <https://reviews.llvm.org/D100073#2702490>, @zoecarver wrote:

>> I don't think this isn't a property of std::indirectly_readable: it's a property of std::indirectly_readable_traits. Please check lines 179-87 in indirectly_readable_traits.compile.pass.cpp for confirmation.
>
> Yeah, I'm glad you have that test. But the standard specifically says `indirectly_readable` and links to the concept. So, I don't think it would hurt to add something like Arthur commented (even though anything with a void type will be rejected during the second check for `iter_reference_t`).

I really don't think this test is necessary. The note is offering commentary on why legacy iterators with a `void` value type have an empty `indirectly_readable_traits`. That transitively means the iterator won't be `indirectly_readable`.



================
Comment at: libcxx/include/__iterator/iterator_traits.h:42
+};
+
 // [iterator.traits]
----------------
zoecarver wrote:
> cjdb wrote:
> > zoecarver wrote:
> > > cjdb wrote:
> > > > zoecarver wrote:
> > > > > Shouldn't these go in `__iterator/concepts.h`?
> > > > No, that will create a circular dependency.
> > > How so? `iterator_traits.h` should be able to include `concepts.h`.
> > `concepts.h` needs to include `iterator_traits.h` because it needs the iterator tag types. I could alternatively put them in their own small header, but //something// needs to be moved around.
> So, no one else needs `__dereferenceable` and friends? Fair enough. I'm fine with this in that case. I'd also support moving the tags into their own header, but I don't feel strongly one way or another (we can always move stuff around after the fact). 
Hmm good point. I'll make a tags header instead.


================
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;
----------------
zoecarver wrote:
> Nit: Maybe `missing_value_type` is more accurate? 
Not sure what this is being applied to (seems to have been displaced).


================
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*>());
----------------
zoecarver wrote:
> Non-blocking: I'm not sure we need to check this and `void*, int`.Same below. 
I do want some rudimentary tests ensuring `void*` isn't indirectly writable.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.writable/indirectly_writable.compile.pass.cpp:58-65
+// <memory>
+static_assert(check_indirectly_writable<std::shared_ptr<int>, int>());
+static_assert(!check_indirectly_writable<std::shared_ptr<void>, int>());
+static_assert(check_indirectly_writable<std::unique_ptr<int>, int>());
+static_assert(!check_indirectly_writable<std::unique_ptr<void>, int>());
+
+// <optional>
----------------
Need to delete these since they're accounted for in their types' respective `iterator_concept_conformance.cpp` files.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100073