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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/random_access_iterator.compile.pass.cpp:31
+// TODO: are we adding cxx20_iterators?
+// static_assert( std::random_access_iterator<cxx20_random_access_iterator<int*> >);
+
----------------
Quuxplusone wrote:
> Mordante wrote:
> > If we don't do this I think we should remove `static_assert(std::random_access_iterator<random_access_iterator<int*> >);` above.
> My impression is that we're not adding `cxx20_anything_iterator` except for `cxx20_input_iterator` (by which we mean "move-only input iterator"). I don't even think `cxx20_forward_iterator` (line 28) exists (nor should it).
> 
> But @Mordante, why do you say you'd remove the test for `std::random_access_iterator<random_access_iterator<int*>>`? I would say that //all// our test iterators should satisfy the appropriate C++20 concepts; if e.g. we have a test `bidirectional_iterator` that is not a `std::bidirectional_iterator`, that's a bug and we should fix it. (AFAIK, all our test iterators //do// satisfy the appropriate C++20 concepts.)
> 
To be clear I prefer to test them both. I only wonder why we should test `static_assert(std::random_access_iterator<random_access_iterator<int*> >);` but don't test `static_assert( std::random_access_iterator<cxx20_random_access_iterator<int*> >);`. If the `cxx20` versions don't exists that would explain it. 


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/random_access_iterator.compile.pass.cpp:75
+
+struct no_plus_equals {
+    typedef std::random_access_iterator_tag iterator_category;
----------------
Quuxplusone wrote:
> Mordante wrote:
> > These classes share a lot of the same boiler-plate. That makes it hard to spot the part being tested. How do you feel about moving the common part to a macro? 
> I agree, but... Each "part being tested" touches a different line of the boilerplate. Do you have an idea of what kind of macro could help here?
> My best offering is, what if you cut-and-pasted the exact same text N times and then //simply added the characters `//`// in order to comment out the one line you wanted to affect in each case. That would still have a lot of the same downsides, but at least it would be a tiny bit clearer what line(s) were intentionally missing, and it would make it easier for a human maintainer to verify that the test "correctly fails" when you comment-in any single one of those lines.
> 
> A very minor improvement would be to use the short names `int*`, `int&`, `int` instead of `pointer`, `reference`, `difference_type`, throughout. That reduces the amount of "stuff" the human eye has to wade through.
If we remove the constructor the classes look the same until `friend bool operator>=(const self, const self y);`. (Look the same since it takes too much effort to validate.) So that could be put in a macro. Then at the interesting part of the class we could use your suggested comments. I think that would make the test easier to maintain.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101316