[libcxx-commits] [PATCH] D129794: [libc++] Fix reverse_iterator::iterator_concept

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/types.compile.pass.cpp:118
   static_assert(std::is_same_v<typename std::reverse_iterator<bidirectional_iterator<char*>>::iterator_concept, std::bidirectional_iterator_tag>);
   static_assert(std::is_same_v<typename std::reverse_iterator<random_access_iterator<char*>>::iterator_concept, std::random_access_iterator_tag>);
+  static_assert(std::is_same_v<typename std::reverse_iterator<cpp20_random_access_iterator<char*>>::iterator_concept, std::random_access_iterator_tag>);
----------------
Mordante wrote:
> huixie90 wrote:
> > Mordante wrote:
> > > Why does this still pass?
> > I guess `random_access_iterator<char*>` is both c++20 `random_access_iterator` and c++17 `random_access_iterator` so the test would pass anyways. the new `cpp20_random_access_iterator` is c++20 `random_access_iterator` but only c++17 `input_iterator`, which would fail the test if the bug weren't fixed
> Thanks for the explanation @huixie90. @philnik if you agree with that assessment, can you use that as comment in the code.
What exactly are you asking for? I don't think it makes a lot of sense to comment about this here, since the pattern "only C++17 input_iterator, but C++20 abc_iterator" exists everywhere in the ranges library.


================
Comment at: libcxx/test/support/test_iterators.h:244
+  template <class U>
+  constexpr cpp20_random_access_iterator(const cpp20_random_access_iterator<U>& u) : it_(u.it_) {}
+
----------------
Mordante wrote:
> Either I overlook it or there are no assignment operators.
There aren't `iterator<U>` assignment operators in any of the test iterators.


================
Comment at: libcxx/test/support/test_iterators.h:292
+  }
+  friend constexpr bool operator!=(const cpp20_random_access_iterator& x, const cpp20_random_access_iterator& y) {
+    return x.it_ != y.it_;
----------------
Mordante wrote:
> Why not use the spaceship operator for these 5 comparisons? The code is only available in C++20.
IIUC defaulting `operator<=>` doesn't work when the members don't have an `operator<=>`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129794