[libcxx-commits] [PATCH] D101316: [libcxx][ranges] Add `random_access_{iterator, range}`.

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

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/containers/views/span.iterators/iterator_concept_conformance.compile.pass.cpp:24
 static_assert(std::bidirectional_iterator<iterator>);
+static_assert(std::random_access_iterator<iterator>);
 static_assert(std::indirectly_writable<iterator, value_type>);
----------------
zoecarver wrote:
> Quuxplusone wrote:
> > It only now occurs to me that it would be useful to verify `std::random_access_iterator<reverse_iterator>` as well.
> > 
> > To keep the tests simple, I think you should add `std::random_access_iterator<reverse_iterator>` only on the containers that are in fact random-access. If `!std::random_access_iterator<iterator>`, then I think we can safely call it obvious that `!std::random_access_iterator<reverse_iterator>`.
> I think we decided as long as we're testing `std::reverse_iterator` it's ok not to test `vector::reverse_iterator` (and other reverse iterators that are known to be type aliases for `std::reverse_iterator`) because we know they're going to be the same. 
> as long as we're testing `std::reverse_iterator`

Are you testing `std::reverse_iterator<std::vector<int>::iterator>`, though? Specifically I do think we should be testing `std::reverse_iterator<std::vector<int>::iterator>` (because it's a `__wrap_iter`) and `std::reverse_iterator<std::vector<bool>::iterator>` (because it's special). And personally I'd do it here, adding to the test for vector iterators (rather than, say, adding to the test for reverse_iterator).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101316