[libc++] Fix reverse_iterator when underlying is c++20 bidirectional_iterator but not Cpp17BidirectionalIterator (#112100)

`reverse_iterator` supports either c++20 `bidirectional_iterator` or
`Cpp17BidirectionalIterator `
http://eel.is/c++draft/reverse.iter.requirements

The current `reverse_iterator` uses `std::prev` in its `operator->`,
which only supports the `Cpp17BidirectionalIterator` properly.

If the underlying iterator is c++20 `bidirectional_iterator` but does
not satisfy the named requirement `Cpp17BidirectionalIterator`,
(examples are `zip_view::iterator`, `flat_map::iterator`), the current
`std::prev` silently compiles but does a no-op and returns the same
iterator back. So `reverse_iterator::operator->` will silently give a
wrong answer.

Even if we fix the behaviour of `std::prev`, at best, we could fail to
compile the code. But this is not ok, because we need to support this
kind of iterators in `reverse_iterator`.

The solution is simply to not use `std::prev`.

---------

Co-authored-by: Louis Dionne <[email protected]>

Author: huixie90

libcxx/include/__iterator/reverse_iterator.h

@@ -136,10 +136,12 @@ class _LIBCPP_TEMPLATE_VIS reverse_iterator
   _LIBCPP_HIDE_FROM_ABI constexpr pointer operator->() const
     requires is_pointer_v<_Iter> || requires(const _Iter __i) { __i.operator->(); }
   {
+    _Iter __tmp = current;
+    --__tmp;
     if constexpr (is_pointer_v<_Iter>) {
-      return std::prev(current);
+      return __tmp;
     } else {
-      return std::prev(current).operator->();
+      return __tmp.operator->();
     }
   }
 #else

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/equal.pass.cpp

@@ -33,6 +33,10 @@ TEST_CONSTEXPR_CXX17 bool tests() {
     test(bidirectional_iterator<const char*>(s), bidirectional_iterator<const char*>(s+1), false);
     test(random_access_iterator<const char*>(s), random_access_iterator<const char*>(s), true);
     test(random_access_iterator<const char*>(s), random_access_iterator<const char*>(s+1), false);
+#if TEST_STD_VER >= 20
+    test(cpp20_random_access_iterator<const char*>(s), cpp20_random_access_iterator<const char*>(s), true);
+    test(cpp20_random_access_iterator<const char*>(s), cpp20_random_access_iterator<const char*>(s + 1), false);
+#endif
     test(s, s, true);
     test(s, s+1, false);
     return true;

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/greater-equal.pass.cpp

@@ -32,6 +32,11 @@ TEST_CONSTEXPR_CXX17 bool tests() {
     test(random_access_iterator<const char*>(s), random_access_iterator<const char*>(s), true);
     test(random_access_iterator<const char*>(s), random_access_iterator<const char*>(s+1), true);
     test(random_access_iterator<const char*>(s+1), random_access_iterator<const char*>(s), false);
+#if TEST_STD_VER >= 20
+    test(cpp20_random_access_iterator<const char*>(s), cpp20_random_access_iterator<const char*>(s), true);
+    test(cpp20_random_access_iterator<const char*>(s), cpp20_random_access_iterator<const char*>(s + 1), true);
+    test(cpp20_random_access_iterator<const char*>(s + 1), cpp20_random_access_iterator<const char*>(s), false);
+#endif
     test(s, s, true);
     test(s, s+1, true);
     test(s+1, s, false);

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/greater.pass.cpp

@@ -32,6 +32,11 @@ TEST_CONSTEXPR_CXX17 bool tests() {
     test(random_access_iterator<const char*>(s), random_access_iterator<const char*>(s), false);
     test(random_access_iterator<const char*>(s), random_access_iterator<const char*>(s+1), true);
     test(random_access_iterator<const char*>(s+1), random_access_iterator<const char*>(s), false);
+#if TEST_STD_VER >= 20
+    test(cpp20_random_access_iterator<const char*>(s), cpp20_random_access_iterator<const char*>(s), false);
+    test(cpp20_random_access_iterator<const char*>(s), cpp20_random_access_iterator<const char*>(s + 1), true);
+    test(cpp20_random_access_iterator<const char*>(s + 1), cpp20_random_access_iterator<const char*>(s), false);
+#endif
     test(s, s, false);
     test(s, s+1, true);
     test(s+1, s, false);

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/less-equal.pass.cpp

@@ -32,6 +32,11 @@ TEST_CONSTEXPR_CXX17 bool tests() {
     test(random_access_iterator<const char*>(s), random_access_iterator<const char*>(s), true);
     test(random_access_iterator<const char*>(s), random_access_iterator<const char*>(s+1), false);
     test(random_access_iterator<const char*>(s+1), random_access_iterator<const char*>(s), true);
+#if TEST_STD_VER >= 20
+    test(cpp20_random_access_iterator<const char*>(s), cpp20_random_access_iterator<const char*>(s), true);
+    test(cpp20_random_access_iterator<const char*>(s), cpp20_random_access_iterator<const char*>(s + 1), false);
+    test(cpp20_random_access_iterator<const char*>(s + 1), cpp20_random_access_iterator<const char*>(s), true);
+#endif
     test(s, s, true);
     test(s, s+1, false);
     test(s+1, s, true);

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/less.pass.cpp

@@ -32,6 +32,11 @@ TEST_CONSTEXPR_CXX17 bool tests() {
     test(random_access_iterator<const char*>(s), random_access_iterator<const char*>(s), false);
     test(random_access_iterator<const char*>(s), random_access_iterator<const char*>(s+1), false);
     test(random_access_iterator<const char*>(s+1), random_access_iterator<const char*>(s), true);
+#if TEST_STD_VER >= 20
+    test(cpp20_random_access_iterator<const char*>(s), cpp20_random_access_iterator<const char*>(s), false);
+    test(cpp20_random_access_iterator<const char*>(s), cpp20_random_access_iterator<const char*>(s + 1), false);
+    test(cpp20_random_access_iterator<const char*>(s + 1), cpp20_random_access_iterator<const char*>(s), true);
+#endif
     test(s, s, false);
     test(s, s+1, false);
     test(s+1, s, true);

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/not-equal.pass.cpp

@@ -33,6 +33,10 @@ TEST_CONSTEXPR_CXX17 bool tests() {
     test(bidirectional_iterator<const char*>(s), bidirectional_iterator<const char*>(s+1), true);
     test(random_access_iterator<const char*>(s), random_access_iterator<const char*>(s), false);
     test(random_access_iterator<const char*>(s), random_access_iterator<const char*>(s+1), true);
+#if TEST_STD_VER >= 20
+    test(cpp20_random_access_iterator<const char*>(s), cpp20_random_access_iterator<const char*>(s), false);
+    test(cpp20_random_access_iterator<const char*>(s), cpp20_random_access_iterator<const char*>(s + 1), true);
+#endif
     test(s, s, false);
     test(s, s+1, true);
     return true;

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/assign.pass.cpp

@@ -59,6 +59,9 @@ TEST_CONSTEXPR_CXX17 bool tests() {
     Derived d;
     test<bidirectional_iterator<Base*> >(bidirectional_iterator<Derived*>(&d));
     test<random_access_iterator<const Base*> >(random_access_iterator<Derived*>(&d));
+#if TEST_STD_VER >= 20
+    test<cpp20_random_access_iterator<const Base*> >(cpp20_random_access_iterator<Derived*>(&d));
+#endif
     test<Base*>(&d);
 
     char c = '\0';

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.default.pass.cpp

@@ -26,6 +26,9 @@ TEST_CONSTEXPR_CXX17 void test() {
 TEST_CONSTEXPR_CXX17 bool tests() {
     test<bidirectional_iterator<const char*> >();
     test<random_access_iterator<char*> >();
+#if TEST_STD_VER >= 20
+    test<cpp20_random_access_iterator<char*> >();
+#endif
     test<char*>();
     test<const char*>();
     return true;

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.iter.pass.cpp

@@ -28,6 +28,9 @@ TEST_CONSTEXPR_CXX17 bool tests() {
     const char s[] = "123";
     test(bidirectional_iterator<const char*>(s));
     test(random_access_iterator<const char*>(s));
+#if TEST_STD_VER >= 20
+    test(cpp20_random_access_iterator<const char*>(s));
+#endif
     test(s);
     return true;
 }

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.reverse_iterator.pass.cpp

@@ -33,6 +33,9 @@ TEST_CONSTEXPR_CXX17 bool tests() {
     Derived d;
     test<bidirectional_iterator<Base*> >(bidirectional_iterator<Derived*>(&d));
     test<random_access_iterator<const Base*> >(random_access_iterator<Derived*>(&d));
+#if TEST_STD_VER >= 20
+    test<cpp20_random_access_iterator<const Base*> >(cpp20_random_access_iterator<Derived*>(&d));
+#endif
     test<Base*>(&d);
     return true;
 }

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.conv/base.pass.cpp

@@ -18,20 +18,28 @@
 #include "test_macros.h"
 #include "test_iterators.h"
 
-TEST_CONSTEXPR_CXX17 bool test() {
-    typedef bidirectional_iterator<int*> Iter;
-    int i = 0;
-    Iter iter(&i);
-    std::reverse_iterator<Iter> const reverse(iter);
-    std::reverse_iterator<Iter>::iterator_type base = reverse.base();
-    assert(base == Iter(&i));
-    return true;
+template <class Iter>
+TEST_CONSTEXPR_CXX17 void test() {
+  int i = 0;
+  Iter iter(&i);
+  std::reverse_iterator<Iter> const reverse(iter);
+  typename std::reverse_iterator<Iter>::iterator_type base = reverse.base();
+  assert(base == Iter(&i));
+}
+
+TEST_CONSTEXPR_CXX17 bool tests() {
+  test<bidirectional_iterator<int*> >();
+  test<random_access_iterator<int*> >();
+#if TEST_STD_VER >= 20
+  test<cpp20_random_access_iterator<int*>>();
+#endif
+  return true;
 }
 
 int main(int, char**) {
-    test();
+  tests();
 #if TEST_STD_VER > 14
-    static_assert(test(), "");
+  static_assert(tests(), "");
 #endif
-    return 0;
+  return 0;
 }

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.elem/arrow.pass.cpp

@@ -24,6 +24,55 @@
 
 #include "test_macros.h"
 
+#if TEST_STD_VER >= 20
+// C++20 bidirectional_iterator that does not satisfy the Cpp17BidirectionalIterator named requirement.
+template <class It>
+class cpp20_bidirectional_iterator_with_arrow {
+  It it_;
+
+public:
+  using iterator_category = std::input_iterator_tag;
+  using iterator_concept  = std::bidirectional_iterator_tag;
+  using value_type        = std::iterator_traits<It>::value_type;
+  using difference_type   = std::iterator_traits<It>::difference_type;
+
+  cpp20_bidirectional_iterator_with_arrow() : it_() {}
+  explicit cpp20_bidirectional_iterator_with_arrow(It it) : it_(it) {}
+
+  decltype(auto) operator*() const { return *it_; }
+
+  auto operator->() const {
+    if constexpr (std::is_pointer_v<It>) {
+      return it_;
+    } else {
+      return it_.operator->();
+    }
+  }
+
+  cpp20_bidirectional_iterator_with_arrow& operator++() {
+    ++it_;
+    return *this;
+  }
+  cpp20_bidirectional_iterator_with_arrow& operator--() {
+    --it_;
+    return *this;
+  }
+  cpp20_bidirectional_iterator_with_arrow operator++(int) { return cpp20_bidirectional_iterator_with_arrow(it_++); }
+  cpp20_bidirectional_iterator_with_arrow operator--(int) { return cpp20_bidirectional_iterator_with_arrow(it_--); }
+
+  friend bool
+  operator==(const cpp20_bidirectional_iterator_with_arrow& x, const cpp20_bidirectional_iterator_with_arrow& y) {
+    return x.it_ == y.it_;
+  }
+  friend bool
+  operator!=(const cpp20_bidirectional_iterator_with_arrow& x, const cpp20_bidirectional_iterator_with_arrow& y) {
+    return x.it_ != y.it_;
+  }
+
+  friend It base(const cpp20_bidirectional_iterator_with_arrow& i) { return i.it_; }
+};
+#endif
+
 class A
 {
     int data_;
@@ -113,6 +162,16 @@ int main(int, char**)
 
     static_assert(it1->get() == gC.get(), "");
   }
+#endif
+#if TEST_STD_VER >= 20
+  {
+    // The underlying iterator models c++20 bidirectional_iterator,
+    // but does not satisfy c++17 BidirectionalIterator named requirement
+    B data[] = {1, 2, 3};
+    cpp20_bidirectional_iterator_with_arrow<B*> iter(data + 3);
+    auto ri = std::make_reverse_iterator(iter);
+    assert(ri->get() == 3);
+  }
 #endif
   {
     ((void)gC);

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.elem/bracket.pass.cpp

@@ -33,6 +33,10 @@ TEST_CONSTEXPR_CXX17 bool tests() {
     const char* s = "1234567890";
     test(random_access_iterator<const char*>(s+5), 4, '1');
     test(random_access_iterator<const char*>(s+5), 0, '5');
+#if TEST_STD_VER >= 20
+    test(cpp20_random_access_iterator<const char*>(s + 5), 4, '1');
+    test(cpp20_random_access_iterator<const char*>(s + 5), 0, '5');
+#endif
     test(s+5, 4, '1');
     test(s+5, 0, '5');
     return true;

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.elem/dereference.pass.cpp

@@ -21,6 +21,7 @@
 #include <cassert>
 
 #include "test_macros.h"
+#include "test_iterators.h"
 
 class A
 {
@@ -47,6 +48,10 @@ int main(int, char**)
 {
     A a;
     test(&a+1, A());
+    test(random_access_iterator<A*>(&a + 1), A());
+#if TEST_STD_VER >= 20
+    test(cpp20_random_access_iterator<A*>(&a + 1), A());
+#endif
 
 #if TEST_STD_VER > 14
     {

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/decrement-assign.pass.cpp

@@ -30,6 +30,9 @@ TEST_CONSTEXPR_CXX17 void test(It i, typename std::iterator_traits<It>::differen
 TEST_CONSTEXPR_CXX17 bool tests() {
     const char* s = "1234567890";
     test(random_access_iterator<const char*>(s+5), 5, random_access_iterator<const char*>(s+10));
+#if TEST_STD_VER >= 20
+    test(cpp20_random_access_iterator<const char*>(s + 5), 5, cpp20_random_access_iterator<const char*>(s + 10));
+#endif
     test(s+5, 5, s+10);
     return true;
 }

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/increment-assign.pass.cpp

@@ -29,7 +29,10 @@ TEST_CONSTEXPR_CXX17 void test(It i, typename std::iterator_traits<It>::differen
 
 TEST_CONSTEXPR_CXX17 bool tests() {
     char const* s = "1234567890";
-    test(random_access_iterator<const char*>(s+5), 5, random_access_iterator<const char*>(s));
+    test(random_access_iterator<const char*>(s + 5), 5, random_access_iterator<const char*>(s));
+#if TEST_STD_VER >= 20
+    test(cpp20_random_access_iterator<const char*>(s + 5), 5, cpp20_random_access_iterator<const char*>(s));
+#endif
     test(s+5, 5, s);
     return true;
 }

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/minus.pass.cpp

@@ -28,7 +28,10 @@ TEST_CONSTEXPR_CXX17 void test(It i, typename std::iterator_traits<It>::differen
 
 TEST_CONSTEXPR_CXX17 bool tests() {
     const char* s = "1234567890";
-    test(random_access_iterator<const char*>(s+5), 5, random_access_iterator<const char*>(s+10));
+    test(random_access_iterator<const char*>(s + 5), 5, random_access_iterator<const char*>(s + 10));
+#if TEST_STD_VER >= 20
+    test(cpp20_random_access_iterator<const char*>(s + 5), 5, cpp20_random_access_iterator<const char*>(s + 10));
+#endif
     test(s+5, 5, s+10);
     return true;
 }

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/plus.pass.cpp

@@ -28,7 +28,10 @@ TEST_CONSTEXPR_CXX17 void test(It i, typename std::iterator_traits<It>::differen
 
 TEST_CONSTEXPR_CXX17 bool tests() {
     const char* s = "1234567890";
-    test(random_access_iterator<const char*>(s+5), 5, random_access_iterator<const char*>(s));
+    test(random_access_iterator<const char*>(s + 5), 5, random_access_iterator<const char*>(s));
+#if TEST_STD_VER >= 20
+    test(cpp20_random_access_iterator<const char*>(s + 5), 5, cpp20_random_access_iterator<const char*>(s));
+#endif
     test(s+5, 5, s);
     return true;
 }

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/postdecrement.pass.cpp

@@ -30,6 +30,9 @@ TEST_CONSTEXPR_CXX17 bool tests() {
     const char* s = "123";
     test(bidirectional_iterator<const char*>(s+1), bidirectional_iterator<const char*>(s+2));
     test(random_access_iterator<const char*>(s+1), random_access_iterator<const char*>(s+2));
+#if TEST_STD_VER >= 20
+    test(cpp20_random_access_iterator<const char*>(s + 1), cpp20_random_access_iterator<const char*>(s + 2));
+#endif
     test(s+1, s+2);
     return true;
 }

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/postincrement.pass.cpp

@@ -30,6 +30,9 @@ TEST_CONSTEXPR_CXX17 bool tests() {
     const char* s = "123";
     test(bidirectional_iterator<const char*>(s+1), bidirectional_iterator<const char*>(s));
     test(random_access_iterator<const char*>(s+1), random_access_iterator<const char*>(s));
+#if TEST_STD_VER >= 20
+    test(cpp20_random_access_iterator<const char*>(s + 1), cpp20_random_access_iterator<const char*>(s));
+#endif
     test(s+1, s);
     return true;
 }

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/predecrement.pass.cpp

@@ -30,6 +30,9 @@ TEST_CONSTEXPR_CXX17 bool tests() {
     const char* s = "123";
     test(bidirectional_iterator<const char*>(s+1), bidirectional_iterator<const char*>(s+2));
     test(random_access_iterator<const char*>(s+1), random_access_iterator<const char*>(s+2));
+#if TEST_STD_VER >= 20
+    test(cpp20_random_access_iterator<const char*>(s + 1), cpp20_random_access_iterator<const char*>(s + 2));
+#endif
     test(s+1, s+2);
     return true;
 }

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/preincrement.pass.cpp

@@ -30,6 +30,9 @@ TEST_CONSTEXPR_CXX17 bool tests() {
     const char* s = "123";
     test(bidirectional_iterator<const char*>(s+1), bidirectional_iterator<const char*>(s));
     test(random_access_iterator<const char*>(s+1), random_access_iterator<const char*>(s));
+#if TEST_STD_VER >= 20
+    test(cpp20_random_access_iterator<const char*>(s + 1), cpp20_random_access_iterator<const char*>(s));
+#endif
     test(s+1, s);
     return true;
 }