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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In general looks good, but some 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>);
----------------
Why does this still pass?


================
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_) {}
+
----------------
Either I overlook it or there are no assignment operators.


================
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_;
----------------
Why not use the spaceship operator for these 5 comparisons? The code is only available in C++20.


================
Comment at: libcxx/test/support/test_iterators.h:925
 // properly, i.e. calling ranges::iter_swap and ranges::iter_move instead of
-// plain swap and std::move 
+// plain swap and std::move
 template <class T>
----------------
While you're at it, please add a full stop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129794