[libc++] Fix reverse_iterator::iterator_concept

[philnik]

Summary

Fixes https://github.com/llvm/llvm-project/issues/56504

[huixie90]

LGTM with nits

libcxx/test/support/test_iterators.h

227

perhaps constrain It to be at least random_access_iterator?

235–240

This is more of FYI and you don't have to follow. To define new C++20 iterators with some level of c++17 backward compatibility support, we typically define:

• iterator_category if the class we define does model c++17 iteartor
• iterator_concept. this is obvious
• value_type for indirectly_readable_traits
• difference_type for incrementable_traits

We actually don't want to define pointer and reference. For C++20 concepts, they are not needed, they are deduced from operator-> (if exists) and operator*. For C++17 backward compatibility, by not defining them, it will trigger the primary template iteartor_traits to properly generate them from operator* and operator-> if they exist.

By defining them it will change the rule of iterator_traits primary template.

So the above is what we typically do when writing proposals for new range adaptors

248

if U itself already models random_access_iterator, it is already default constructible. If not, then perhaps some of the member functions will be broken. Do we need to check here?

253

Again you don't have to do. A really common category of iterators that model c++20 randam_access_iterator but only c++17 input_iterator are iterators with prvalue reference type, such as iota, transform iterator with F return by value. If I wrote this class, I'd simulate that. but you don't have to

309

This class defined pointer to be It, but not defining operator-> , which is incorrect. I'd recommend just not define pointer since operator-> isn't required

311

perhaps forward iter_move and iter_swap as well in case this class is used in the future by some other tests?

316

perhaps also static_assert the iterator_traits::iterator_category is not derived from random access tag?

[philnik]

• Address comments

libcxx/test/support/test_iterators.h

235–240

Thanks for the explanation!

248

Nope, it's just a copy-paste.

311

I don't know if we will ever use this with move advanced iterators. I'd rather keep it at the minimum for random_access_iterators for now. We can still update it later if we want.

316

I don't really see the value in that. You can just look at the typedef and know that it's the case. Verifying that random_access_iterator is satisfied is really hard OTOH.

[Mordante]

In general looks good, but some comments.

libcxx/test/std/iterators/predef.iterators/reverse.iterators/types.compile.pass.cpp

118–119

Why does this still pass?

libcxx/test/support/test_iterators.h

244

Either I overlook it or there are no assignment operators.

292

Why not use the spaceship operator for these 5 comparisons? The code is only available in C++20.

950

While you're at it, please add a full stop.

[huixie90]

libcxx/test/std/iterators/predef.iterators/reverse.iterators/types.compile.pass.cpp

118–119

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

[Mordante]

libcxx/test/std/iterators/predef.iterators/reverse.iterators/types.compile.pass.cpp

118–119

Thanks for the explanation @huixie90. @philnik if you agree with that assessment, can you use that as comment in the code.

[ldionne]

LGTM in essence, leaving final approval to others who chimed in on this. We should go through our archetypes and make sure they are as minimal as possible, i.e. we should probably have a cpp20_xxxxx and a cpp17_xxxxx version for each iterator that has differences between the two standards. Not for this patch, of course.

[philnik]

• Address comments

libcxx/test/std/iterators/predef.iterators/reverse.iterators/types.compile.pass.cpp

118–119

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.

libcxx/test/support/test_iterators.h

244

There aren't iterator<U> assignment operators in any of the test iterators.

292

IIUC defaulting operator<=> doesn't work when the members don't have an operator<=>.

[Mordante]

LGTM!

libcxx/include/__iterator/reverse_iterator.h

 public:
     using iterator_type = _Iter;
 
     using iterator_category = _If<__is_cpp17_random_access_iterator<_Iter>::value,
                                   random_access_iterator_tag,
                                   typename iterator_traits<_Iter>::iterator_category>;
     using pointer = typename iterator_traits<_Iter>::pointer;
 #if _LIBCPP_STD_VER > 17
-    using iterator_concept = _If<__is_cpp17_random_access_iterator<_Iter>::value,
-                                  random_access_iterator_tag,
-                                  bidirectional_iterator_tag>;
+    using iterator_concept = _If<random_access_iterator<_Iter>, random_access_iterator_tag, bidirectional_iterator_tag>;
     using value_type = iter_value_t<_Iter>;
     using difference_type = iter_difference_t<_Iter>;
     using reference = iter_reference_t<_Iter>;
 #else
     using value_type = typename iterator_traits<_Iter>::value_type;
     using difference_type = typename iterator_traits<_Iter>::difference_type;
     using reference = typename iterator_traits<_Iter>::reference;
 #endif

libcxx/test/std/iterators/predef.iterators/reverse.iterators/types.compile.pass.cpp

 void test_all() {
   test<bidirectional_iterator<char*> >();
   test<random_access_iterator<char*> >();
   test<char*>();
 
 #if TEST_STD_VER > 17
   test<contiguous_iterator<char*>>();
   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>);
   static_assert(std::is_same_v<typename std::reverse_iterator<contiguous_iterator<char*>>::iterator_concept, std::random_access_iterator_tag>);
   static_assert(std::is_same_v<typename std::reverse_iterator<char*>::iterator_concept, std::random_access_iterator_tag>);
 #endif
 }

libcxx/test/support/test_iterators.h

public:
 
     friend TEST_CONSTEXPR It base(const random_access_iterator& i) { return i.it_; }
 
     template <class T>
     void operator,(T const &) = delete;
 };
 
 #if TEST_STD_VER > 17
+
+template <std::random_access_iterator It>
+class cpp20_random_access_iterator {
+  It it_;
+
+  template <std::random_access_iterator>
+  friend class cpp20_random_access_iterator;
+
+public:
+  using iterator_category = std::input_iterator_tag;
+  using iterator_concept  = std::random_access_iterator_tag;
+  using value_type        = typename std::iterator_traits<It>::value_type;
+  using difference_type   = typename std::iterator_traits<It>::difference_type;
+
+  constexpr cpp20_random_access_iterator() : it_() {}
+  constexpr explicit cpp20_random_access_iterator(It it) : it_(it) {}
+
+  template <class U>
+  constexpr cpp20_random_access_iterator(const cpp20_random_access_iterator<U>& u) : it_(u.it_) {}
+
+  template <class U>
+  constexpr cpp20_random_access_iterator(cpp20_random_access_iterator<U>&& u) : it_(u.it_) {
+    u.it_ = U();
+  }
+
+  constexpr decltype(auto) operator*() const { return *it_; }
+  constexpr decltype(auto) operator[](difference_type n) const { return it_[n]; }
+
+  constexpr cpp20_random_access_iterator& operator++() {
+    ++it_;
+    return *this;
+  }
+  constexpr cpp20_random_access_iterator& operator--() {
+    --it_;
+    return *this;
+  }
+  constexpr cpp20_random_access_iterator operator++(int) { return cpp20_random_access_iterator(it_++); }
+  constexpr cpp20_random_access_iterator operator--(int) { return cpp20_random_access_iterator(it_--); }
+
+  constexpr cpp20_random_access_iterator& operator+=(difference_type n) {
+    it_ += n;
+    return *this;
+  }
+  constexpr cpp20_random_access_iterator& operator-=(difference_type n) {
+    it_ -= n;
+    return *this;
+  }
+  friend constexpr cpp20_random_access_iterator operator+(cpp20_random_access_iterator x, difference_type n) {
+    x += n;
+    return x;
+  }
+  friend constexpr cpp20_random_access_iterator operator+(difference_type n, cpp20_random_access_iterator x) {
+    x += n;
+    return x;
+  }
+  friend constexpr cpp20_random_access_iterator operator-(cpp20_random_access_iterator x, difference_type n) {
+    x -= n;
+    return x;
+  }
+  friend constexpr difference_type operator-(cpp20_random_access_iterator x, cpp20_random_access_iterator y) {
+    return x.it_ - y.it_;
+  }
+
+  friend constexpr bool operator==(const cpp20_random_access_iterator& x, const cpp20_random_access_iterator& y) {
+    return x.it_ == y.it_;
+  }
+  friend constexpr bool operator!=(const cpp20_random_access_iterator& x, const cpp20_random_access_iterator& y) {
+    return x.it_ != y.it_;
+  }
+  friend constexpr bool operator<(const cpp20_random_access_iterator& x, const cpp20_random_access_iterator& y) {
+    return x.it_ < y.it_;
+  }
+  friend constexpr bool operator<=(const cpp20_random_access_iterator& x, const cpp20_random_access_iterator& y) {
+    return x.it_ <= y.it_;
+  }
+  friend constexpr bool operator>(const cpp20_random_access_iterator& x, const cpp20_random_access_iterator& y) {
+    return x.it_ > y.it_;
+  }
+  friend constexpr bool operator>=(const cpp20_random_access_iterator& x, const cpp20_random_access_iterator& y) {
+    return x.it_ >= y.it_;
+  }
+
+  friend constexpr It base(const cpp20_random_access_iterator& i) { return i.it_; }
+
+  template <class T>
+  void operator,(T const&) = delete;
+};
+
+static_assert(std::random_access_iterator<cpp20_random_access_iterator<int*>>);
+
 template <class It>
 class contiguous_iterator
 {
     static_assert(std::is_pointer_v<It>, "Things probably break in this case");
 
     It it_;
 
     template <class U> friend class contiguous_iterator;
 public:

 // ======================================================================
 // Proxy that can wrap a value or a reference. It simulates C++23's tuple
 // but simplified to just hold one argument.
 // Note that unlike tuple, this class deliberately doesn't have special handling
 // of swap to cause a compilation error if it's used in an algorithm that relies
 // on plain swap instead of ranges::iter_swap.
 // This class is useful for testing that if algorithms support proxy iterator
 // 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>
 struct Proxy;
 
 template <class T>
 inline constexpr bool IsProxy = false;
 
 template <class T>
 inline constexpr bool IsProxy<Proxy<T>> = true;