[libcxx-commits] [PATCH] D101396: [libcxx][ranges] Add `contiguous_iterator`.

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

LGTM % comments



================
Comment at: libcxx/include/__iterator/concepts.h:16
 #include <__iterator/iter_move.h>
 #include <__iterator/incrementable_traits.h>
 #include <__iterator/iterator_traits.h>
----------------
At some point: Alphabetize.


================
Comment at: libcxx/test/std/containers/sequences/vector.bool/iterator_concept_conformance.compile.pass.cpp:27
+static_assert(!std::contiguous_iterator<iterator>);
+static_assert(!std::contiguous_iterator<reverse_iterator>);
 static_assert(!std::indirectly_writable<iterator, value_type>);
----------------
cjdb wrote:
> zoecarver wrote:
> > Just want to confirm this is correct: `vector<bool>` is not a contiguous container (while normal vector is). 
> Correct.
Please keep `static_assert(std::random_access_iterator<reverse_iterator>);` (just //add// the new negative asserts).


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp:14
+// template<class T>
+// concept random_access_iterator;
+
----------------
concept contiguous_iterator;


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp:45-49
+    friend bool operator==(const self, const self y);
+    friend bool operator< (const self, const self y);
+    friend bool operator<=(const self, const self y);
+    friend bool operator> (const self, const self y);
+    friend bool operator>=(const self, const self y);
----------------
(A) this could be `operator<=>`?
(B) pass-by-const-value alert! I think you wanted pass-by-const-reference.
Ditto throughout.


================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/iterator_concept_conformance.compile.pass.cpp:42
 static_assert(common_reverse_iterator_checks<reverse_contiguous_iterator>());
-static_assert(std::random_access_iterator<reverse_contiguous_iterator>);
+static_assert(!std::contiguous_iterator<reverse_contiguous_iterator>);
 static_assert(std::sized_sentinel_for<reverse_contiguous_iterator, reverse_contiguous_iterator>);
----------------
Please keep the assert for
```
static_assert(std::random_access_iterator<reverse_contiguous_iterator>);
```
(just //add// the new negative assert). This also applies anywhere else in this PR that you removed a positive assert without replacing it with a stronger positive assert (if any such places exist).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101396