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

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

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
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*> >);
+
----------------
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.)



================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/random_access_iterator.compile.pass.cpp:46
+
+    simple_random_access_iterator();
+
----------------
`explicit` (because all ctors should be explicit), or else remove (because the implicitly generated one seems OK enough).


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/random_access_iterator.compile.pass.cpp:54
+    friend bool operator> (const self, const self y);
+    friend bool operator>=(const self, const self y);
+
----------------
Mordante wrote:
> Is there a reason to only name the second argument?
Not to mention, passing "by const value" is typically a bug. I think `const self&` is meant (or, if it wasn't, it should have been). Also, since this test is C++20-only, how would we feel about a single `friend std::strong_ordering operator<=>(const self&, const self&);`? (Not defaulted, just in case that mattered.)

Personally I would not object to pass-by-value, e.g. replacing the current (or-should-be)
    friend int operator-(const self&, const self&);
with
    friend int operator-(self, self);
This wouldn't be as realistic, but it would reduce the amount of "stuff" to wade through.


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


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/random_access_iterator.compile.pass.cpp:108
+};
+static_assert(std::forward_iterator<no_plus_equals>);
+static_assert(!std::random_access_iterator<no_plus_equals>);
----------------
In fact, bidirectional! Right? (Ditto throughout.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101316