Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Oct 19, 2024

Conversation

huixie90
Copy link
Contributor

@huixie90 huixie90 commented Oct 12, 2024

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.

…l_iterator but not c++17 BidirectionalIterator
@huixie90 huixie90 requested a review from a team as a code owner October 12, 2024 15:22
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Oct 12, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 12, 2024

@llvm/pr-subscribers-libcxx

Author: Hui (huixie90)

Changes

Patch is 29.57 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/112100.diff

26 Files Affected:

  • (modified) libcxx/include/__iterator/reverse_iterator.h (+4-2)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/equal.pass.cpp (+4)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/greater-equal.pass.cpp (+5)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/greater.pass.cpp (+5)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/less-equal.pass.cpp (+5)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/less.pass.cpp (+5)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/not-equal.pass.cpp (+4)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/assign.pass.cpp (+3)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.default.pass.cpp (+3)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.iter.pass.cpp (+3)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.reverse_iterator.pass.cpp (+3)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.conv/base.pass.cpp (+13-5)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.elem/arrow.pass.cpp (+58)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.elem/bracket.pass.cpp (+4)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.elem/dereference.pass.cpp (+5)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/decrement-assign.pass.cpp (+3)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/increment-assign.pass.cpp (+3)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/minus.pass.cpp (+3)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/plus.pass.cpp (+3)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/postdecrement.pass.cpp (+3)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/postincrement.pass.cpp (+3)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/predecrement.pass.cpp (+3)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/preincrement.pass.cpp (+3)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/make_reverse_iterator.pass.cpp (+15-5)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/minus.pass.cpp (+47-28)
  • (modified) libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/plus.pass.cpp (+3)
diff --git a/libcxx/include/__iterator/reverse_iterator.h b/libcxx/include/__iterator/reverse_iterator.h
index 50c0f21eaa286b..5e88d86ad5e9b2 100644
--- a/libcxx/include/__iterator/reverse_iterator.h
+++ b/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
diff --git a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/equal.pass.cpp b/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/equal.pass.cpp
index fcf8d88fcf62be..b5cb1324a59248 100644
--- a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/equal.pass.cpp
+++ b/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;
diff --git a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/greater-equal.pass.cpp b/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/greater-equal.pass.cpp
index fdcd02abb0d8ed..3c37dce13dc6af 100644
--- a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/greater-equal.pass.cpp
+++ b/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);
diff --git a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/greater.pass.cpp b/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/greater.pass.cpp
index dce331e519646f..288691b7147757 100644
--- a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/greater.pass.cpp
+++ b/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);
diff --git a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/less-equal.pass.cpp b/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/less-equal.pass.cpp
index e9cea6250a7645..f7babf1ff48d20 100644
--- a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/less-equal.pass.cpp
+++ b/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);
diff --git a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/less.pass.cpp b/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/less.pass.cpp
index b66147cf3a03c3..6f3d82c23430fb 100644
--- a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/less.pass.cpp
+++ b/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);
diff --git a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/not-equal.pass.cpp b/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/not-equal.pass.cpp
index 37a6ff1302ce77..1b921ace4a67f0 100644
--- a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/not-equal.pass.cpp
+++ b/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;
diff --git a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/assign.pass.cpp b/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/assign.pass.cpp
index 0e5123a49e2b56..f9d2efa7c2a8cc 100644
--- a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/assign.pass.cpp
+++ b/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';
diff --git a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.default.pass.cpp b/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.default.pass.cpp
index fcb96de91d1a02..90047b19f5a63a 100644
--- a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.default.pass.cpp
+++ b/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;
diff --git a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.iter.pass.cpp b/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.iter.pass.cpp
index 801b2cf879ce5b..72e77b08564219 100644
--- a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.iter.pass.cpp
+++ b/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;
 }
diff --git a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.reverse_iterator.pass.cpp b/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.reverse_iterator.pass.cpp
index 8f315e83f6d7b4..fa967b45b1d9f8 100644
--- a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.reverse_iterator.pass.cpp
+++ b/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;
 }
diff --git a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.conv/base.pass.cpp b/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.conv/base.pass.cpp
index 4fb33f54260457..ded372a32b0b3a 100644
--- a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.conv/base.pass.cpp
+++ b/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;
+template <class Iter>
+TEST_CONSTEXPR_CXX17 void test() {
     int i = 0;
     Iter iter(&i);
     std::reverse_iterator<Iter> const reverse(iter);
-    std::reverse_iterator<Iter>::iterator_type base = reverse.base();
+    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;
 }
diff --git a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.elem/arrow.pass.cpp b/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.elem/arrow.pass.cpp
index 15d18d9145ef0c..ac862ef1a2492c 100644
--- a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.elem/arrow.pass.cpp
+++ b/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.elem/arrow.pass.cpp
@@ -24,6 +24,54 @@
 
 #include "test_macros.h"
 
+#if TEST_STD_VER >= 20
+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 +161,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);
diff --git a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.elem/bracket.pass.cpp b/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.elem/bracket.pass.cpp
index 37a857ceefa83d..59bea3a6235ba4 100644
--- a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.elem/bracket.pass.cpp
+++ b/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;
diff --git a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.elem/dereference.pass.cpp b/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.elem/dereference.pass.cpp
index 292c6da9a7733e..f568a918a6a835 100644
--- a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.elem/dereference.pass.cpp
+++ b/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
     {
diff --git a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/decrement-assign.pass.cpp b/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/decrement-assign.pass.cpp
index 8c83ec1e9389f9..dd5fd2c88645ad 100644
--- a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/decrement-assign.pass.cpp
+++ b/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;
 }
diff --git a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/increment-assign.pass.cpp b/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/increment-assign.pass.cpp
index e32fac9fc24fe1..74ab3f96189233 100644
--- a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/increment-assign.pass.cpp
+++ b/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/increment-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() {
     char const* s = "1234567890";
     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;
 }
diff --git a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/minus.pass.cpp b/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/minus.pass.cpp
index f2474dd7669f2c..816c11c65d2c48 100644
--- a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/minus.pass.cpp
+++ b/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/minus.pass.cpp
@@ -29,6 +29,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;
 }
diff --git a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/plus.pass.cpp b/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/plus.pass.cpp
index 5673425e796757..72640cd0ec80c3 100644
--- a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/plus.pass.cpp
+++ b/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nav/plus.pass.cpp
@@ -29,6 +29,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));
+#if TEST_STD_VER >= 20
+    ...
[truncated]

Copy link

github-actions bot commented Oct 12, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@huixie90 huixie90 changed the title [libc++] Fix reverse_iterator when underlying is c++20 bidirectional_iterator but not c++17 BidirectionalIterator [libc++] Fix reverse_iterator when underlying is c++20 bidirectional_iterator but not Cpp17BidirectionalIterator Oct 12, 2024
@huixie90 huixie90 changed the title [libc++] Fix reverse_iterator when underlying is c++20 bidirectional_iterator but not Cpp17BidirectionalIterator [libc++] Fix reverse_iterator when underlying is c++20 bidirectional_iterator but not Cpp17BidirectionalIterator Oct 12, 2024
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also appears to reduce inlining burden on compilers. LGTM once test files get clang-formatted.

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a nitpick. Thanks for fixing!

…everse.iter.elem/arrow.pass.cpp

Co-authored-by: Louis Dionne <[email protected]>
@huixie90 huixie90 merged commit 1bbf3a3 into llvm:main Oct 19, 2024
62 checks passed
EricWF pushed a commit to efcs/llvm-project that referenced this pull request Oct 22, 2024
…al_iterator` but not `Cpp17BidirectionalIterator` (llvm#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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants