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

Mon May 3 15:43:53 PDT 2021

Quuxplusone added inline comments.

================
Comment at: libcxx/test/libcxx/iterators/iterator.concepts/iterator.concept.random.access/subsumption.compile.pas.cpp:21
+template<std::bidirectional_iterator I>
+requires std::derived_from<std::_ITER_CONCEPT<I>, std::random_access_iterator_tag>
+[[nodiscard]] constexpr bool check_subsumption() {
----------------
cjdb wrote:
> Per offline discussion, please remove, and move to `test/std/...`.
Also, this file's name ends in `.pas.cpp`.
I sense another automated CI step in our future...

================
Comment at: libcxx/test/libcxx/iterators/iterator.concepts/iterator.concept.random.access/subsumption.compile.pas.cpp:27
+template<std::random_access_iterator>
+[[nodiscard]] constexpr bool check_subsumption() {
+  return true;
----------------
Whatever else you're shuffling around here, please also remove the `[[nodiscard]]` cruft.

================
Comment at: libcxx/test/std/containers/associative/multiset/iterator_concept_conformance.compile.pass.cpp:38
 static_assert(std::bidirectional_iterator<const_iterator>);
+static_assert(!std::random_access_iterator<iterator>);
 static_assert(!std::indirectly_writable<const_iterator, value_type>);
----------------
`const_iterator` (and please triple-check, throughout)

================
Comment at: libcxx/test/std/containers/sequences/vector.bool/iterator_concept_conformance.compile.pass.cpp:26
 static_assert(std::bidirectional_iterator<iterator>);
+static_assert(std::random_access_iterator<iterator>);
 static_assert(!std::indirectly_writable<iterator, value_type>);
----------------
For my own curiosity: I thought the new definition of `forward_iterator` was that its `reference` type needed to be `value_type&`. How does `vector<bool>::iterator` still manage to qualify as a `random_access_iterator`, then?
(This is presumably not a bug with this patch; I ask only for information.)

================
Comment at: libcxx/test/std/containers/views/range_concept_conformance.compile.pass.cpp:20

 using range = std::span<int>;
 namespace stdr = std::ranges;
----------------
Presumably in a separate PR, I'd like to see some tests for `span<const int>`.
(For consistency with the existing tests, I mean. This is not intended as my ringing endorsement of the current testing strategy. ;))

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

================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/random_access_iterator.compile.pass.cpp:68
+template<class Parent>
+struct simple_bidirectional_iterator {
+    typedef std::random_access_iterator_tag iterator_category;
----------------
Bikeshed: I'd prefer to call this `bidirectional_iterator_base`... or `random_access_base` since its category is actually intentionally `random_access_iterator_tag`... or heck, just `common_base`!

Point is, it is //not// parallel to `simple_random_access_iterator` below, and its name should reflect that not-parallel-ness.

And, I mean, technically `Parent` should be named `Child`, but I get it. ;) I sometimes use the name `Crtp` for that parameter.

================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/random_access_iterator.compile.pass.cpp:101
+    difference_type operator-(const self&) const;
+
+    reference operator[](difference_type n) const;
----------------
Throughout (from line 93 down): The blank lines on 93, 97, 101, 109, 113, 117,... don't seem semantically useful. I'd eliminate them.

================
Comment at: libcxx/test/std/ranges/range.refinements/random_access_range.compile.pass.cpp:36
+
+static_assert(!check_range<cpp20_input_iterator>());
+static_assert(!check_range<forward_iterator>());
----------------
cjdb wrote:
> Please also add `cpp17_input_iterator` wherever you check `cpp20_input_iterator`, for completeness' sake.
+1

================
Comment at: libcxx/test/std/ranges/range.refinements/subsumption.compile.pass.cpp:76
+template<std::ranges::bidirectional_range R>
+requires std::ranges::random_access_range<std::ranges::iterator_t<R> >
+[[nodiscard]] constexpr bool check_random_access_range_subsumption() {
----------------
This line is clearly intended to say `random_access_iterator` instead of `random_access_range`.

TODO: find a less error-prone way to write these tests. Right now we're just checking that this function //isn't the best candidate//, which is fairly obviously the case — even if it //were// viable. We should find a way to do this that doesn't quietly fall into a false negative when there's a problem with the cut-and-paste. (Or, handcraft these test cases more carefully.)

================
Comment at: libcxx/test/std/ranges/range.refinements/subsumption.compile.pass.cpp:83
+requires true
+[[nodiscard]] constexpr bool check_random_access_range_subsumption() {
+  return true;
----------------
Throughout: Remove `[[nodiscard]]` cruft.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101316