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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Some remarks, mostly minor. But I really would like to see it pass a CI run.



================
Comment at: libcxx/test/std/containers/sequences/forwardlist/range_concept_conformance.compile.pass.cpp:28
 static_assert(!stdr::bidirectional_range<range>);
+static_assert(stdr::random_access_range<range>);
 
----------------
I don't think `forward_list` has a `random_access_iterator`.


================
Comment at: libcxx/test/std/containers/sequences/list/range_concept_conformance.compile.pass.cpp:27
 static_assert(stdr::bidirectional_range<range>);
+static_assert(stdr::random_access_range<range>);
 
----------------
I don't think `list` has a `random_access_iterator`.


================
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*> >);
+
----------------
If we don't do this I think we should remove `static_assert(std::random_access_iterator<random_access_iterator<int*> >);` above.


================
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);
+
----------------
Is there a reason to only name the second argument?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101316