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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb 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>);
----------------
Quuxplusone wrote:
> cjdb wrote:
> > cjdb wrote:
> > > Quuxplusone wrote:
> > > > 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).
> > > > 
> > > > 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>`.
> > > 
> > > This should be in the `reverse_iterator` conformance test, and can probably be just `reverse_iterator<reverse_iterator<int*>>`. This opinion is neutral for addition, strongly in favour for placement.
> > > 
> > > As for `__wrap_iter`/`vector<bool>::iterator`, I'm not convinced that's necessary, since the conformance checks confirms `reverse_iterator<some_random_access_iterator>` is a random-access iterator. This opinion is weakly-against/borderline neutral for addition and strongly in agreement with Arthur for placement.
> > > As for `__wrap_iter`/`vector<bool>::iterator`, I'm not convinced that's necessary, since the conformance checks confirms `reverse_iterator<some_random_access_iterator>` is a random-access iterator.
> > 
> > I didn't finish this thought. We confirm that `vector::iterator` is a `random_access_iterator` //and// we confirm that `reverse_iterator` plays well with a canonical random-access iterator, so I'm not convinced this is exploring anything new.
> > 
> > 
> It'd be exploring `__wrap_iter` specifically, but through a test that would be legal in `test/std/`. (Sometimes known as "whitebox testing": we'll be poking at the outside of the box, but in a way that we happen to know will exercise one specific implementation detail.)
> 
> Orthogonally to `__wrap_iter`, I still maintain that `vector<bool>::iterator` is //not// a canonical random-access iterator, and therefore testing `reverse_iterator<random_access_iterator>` does //not// really tell us whether `reverse_iterator<vector<bool>::iterator>` is going to work or not.
> It'd be exploring `__wrap_iter` specifically, but through a test that would be legal in `test/std/`. (Sometimes known as "whitebox testing": we'll be poking at the outside of the box, but in a way that we happen to know will exercise one specific implementation detail.)

What //new// territory does `reverse_iterator<__wrap_iter>` explore that `random_access_iterator<__wrap_iter>` and `random_access_iterator<reverse_iterator<int*>>` doesn't already cover? I'm all for expanding coverage, but I'd very much like to understand what you're expanding to.

> Orthogonally to `__wrap_iter`, I still maintain that `vector<bool>::iterator` is //not// a canonical random-access iterator, and therefore testing `reverse_iterator<random_access_iterator>` does //not// really tell us whether `reverse_iterator<vector<bool>::iterator>` is going to work or not.

You can believe this all you like, but the definition of iterators was revised specifically to let proxy iterators satisfy concepts stronger than `input_iterator`. As of C++20, `vector<bool>::iterator` is a random-access iterator. We confirm as much in `libcxx/test/std/containers/sequences/vector.bool/iterator_concept_conformance.compile.pass.cpp`. I'm not sure where you're drawing your objection from.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101316