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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
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<
----------------
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100073