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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
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;
----------------
Quuxplusone wrote:
> Throughout (from line 93 down): The blank lines on 93, 97, 101, 109, 113, 117,... don't seem semantically useful. I'd eliminate them.
At least for me, it's helpful to break things up in my mind. "These are the plus operators, these are the minus operators, this is the subscript." If they were all next to each other, it would be easy for something to get lost (and it's especially important not to let things get lost when the whole point of these objects is to remove //one, specific// method). 

Also, not that this is a good reason on it's own, but this mimics how other iterators are defined (such as `test::random_access_iterator`).


================
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() {
----------------
Quuxplusone wrote:
> 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.)
:/

Unfortunately a lot of thought has gone into these tests. I'd 100% be open, if anyone has a better way to test them, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101316