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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added a comment.

In D100073#2692629 <https://reviews.llvm.org/D100073#2692629>, @ldionne wrote:

> Actually, while looking at a different review, I stumbled upon this note: http://eel.is/c++draft/iterator.assoc.types#readable.traits-note-1
>
>> Some legacy output iterators define a nested type named `value_­type` that is an alias for `void`. These types are not `indirectly_­readable` and have no associated value types.
>
> I don't think we have a test for that here, so we should add one if we don't. Then there's also this note: http://eel.is/c++draft/iterator.assoc.types#readable.traits-note-2, but I see we do have a test for that (great).
>
> Requesting changes so this shows up correctly in the queue.

Are you sure? `test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp:179-87` already check for a `void` `value_type`/`element_type`. It doesn't really matter whether or not the type is an iterator.



================
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;
----------------
ldionne wrote:
> 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.
How about now?


================
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>());
+
----------------
zoecarver wrote:
> This looks very similar to `iter_move_mismatch`. What's the difference? 
It's a distinct type (one per test is necessary since `common_reference` needs to resolve as a different type).


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.readable/indirectly_readable.compile.pass.cpp:308
+static_assert(
+    check_indirectly_readable<std::unordered_multiset<int, int>::iterator>());
+static_assert(check_indirectly_readable<
----------------
Quuxplusone wrote:
> zoecarver wrote:
> > Only one template param. 
> I can't follow all the template metaprogramming on lines 152-175, and lines 184-283 are obviously overkill (//all// iterator types are indirectly readable, by definition; we don't need a million header dependencies to prove that). The use of `check_indirectly_readable<T>()` in place of `std::indirectly_readable<T>` is also a bit obfuscatory; I would prefer to see the concept tested directly.
> 
> One test I'd like to see added is
> ```
> struct NoConst { int& operator*(); };
> static_assert(!std::indirectly_readable<NoConst>);
> ```
> This is the sort of thing that (A) may come up in real code, and (B) may break under maintenance, if someone changes `(const _In __in)` to `(_In __in)` on line 2507.  (`requires`-expression parameters are frequently treated the same as function parameters, and one would never const-qualify a function parameter; but here, the `const` keyword on the requires-parameter is actually the key to conformance. And we have no test for it.)
> I can't follow all the template metaprogramming on lines 152-175

`common_reference_t<iter_ref4, iter_rvalue_ref>` is `iter_rvalue_ref` and `common_reference<iter_ref4 const&, iter_rvalue_ref&&>::type` doesn't exist.

> lines 184-283 are obviously overkill (//all// iterator types are indirectly readable, by definition; we don't need a million header dependencies to prove that)

This ensures conformance, which is a non-obvious thing to confirm. A full review of the tests would have revealed that not "//all//" iterators are indirectly readable, by definition. I'll leave it as an exercise for you to find those that don't satisfy the concept.

However, at your insistence, I've removed them from this file.

> The use of `check_indirectly_readable<T>()` in place of `std::indirectly_readable<T>` is also a bit obfuscatory; I would prefer to see the concept tested directly.

What is being obfuscated?

> One test I'd like to see added is
> ```
> struct NoConst { int& operator*(); };
> static_assert(!std::indirectly_readable<NoConst>);
> ```
> This is the sort of thing that (A) may come up in real code, and (B) may break under maintenance, if someone changes `(const _In __in)` to `(_In __in)` on line 2507. (`requires`-expression parameters are frequently treated the same as function parameters, and one would never const-qualify a function parameter; but here, the const keyword on the requires-parameter is actually the key to conformance. And we have no test for it.)

Added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100073