[libc++][spaceship] Implements X::iterator container requirements. #99343

mordante · 2024-07-17T15:46:25Z

This implements the requirements for the container iterator requirements for array, deque, vector, and vector<bool>.

Implements:

LWG3352 strong_equality isn't a thing

Implements parts of:

P1614R2 The Mothership has Landed

Fixes: #62486

llvmbot · 2024-07-17T15:46:58Z

@llvm/pr-subscribers-libcxx

Author: Mark de Wever (mordante)

Changes

This implements the requirements for the container iterator requirements for array, deque, vector, and vector<bool>.

Implements:

LWG3352 strong_equality isn't a thing

Implements parts of:

P1614R2 The Mothership has Landed

Fixes: #62486

Full diff: https://github.com/llvm/llvm-project/pull/99343.diff

9 Files Affected:

(modified) libcxx/docs/Status/Cxx20Issues.csv (+1-1)
(modified) libcxx/docs/Status/SpaceshipProjects.csv (+1-1)
(modified) libcxx/include/__bit_reference (+14)
(modified) libcxx/include/__iterator/wrap_iter.h (+19)
(modified) libcxx/include/deque (+12)
(modified) libcxx/test/std/containers/sequences/array/iterators.pass.cpp (+18)
(modified) libcxx/test/std/containers/sequences/deque/iterators.pass.cpp (+9)
(modified) libcxx/test/std/containers/sequences/vector.bool/iterators.pass.cpp (+9)
(modified) libcxx/test/std/containers/sequences/vector/iterators.pass.cpp (+9-1)

diff --git a/libcxx/docs/Status/Cxx20Issues.csv b/libcxx/docs/Status/Cxx20Issues.csv
index 1a40a4472a405..8a431c922a2d9 100644
--- a/libcxx/docs/Status/Cxx20Issues.csv
+++ b/libcxx/docs/Status/Cxx20Issues.csv
@@ -264,7 +264,7 @@
 "`3349 <https://wg21.link/LWG3349>`__","Missing ``__cpp_lib_constexpr_complex``\  for P0415R1","Prague","|Complete|","16.0"
 "`3350 <https://wg21.link/LWG3350>`__","Simplify return type of ``lexicographical_compare_three_way``\ ","Prague","|Complete|","17.0","|spaceship|"
 "`3351 <https://wg21.link/LWG3351>`__","``ranges::enable_safe_range``\  should not be constrained","Prague","|Complete|","15.0","|ranges|"
-"`3352 <https://wg21.link/LWG3352>`__","``strong_equality``\  isn't a thing","Prague","|Nothing To Do|","","|spaceship|"
+"`3352 <https://wg21.link/LWG3352>`__","``strong_equality``\  isn't a thing","Prague","|Complete|","19.0","|spaceship|"
 "`3354 <https://wg21.link/LWG3354>`__","``has_strong_structural_equality``\  has a meaningless definition","Prague","|Nothing To Do|","","|spaceship|"
 "`3355 <https://wg21.link/LWG3355>`__","The memory algorithms should support move-only input iterators introduced by P1207","Prague","|Complete|","15.0","|ranges|"
 "`3356 <https://wg21.link/LWG3356>`__","``__cpp_lib_nothrow_convertible``\  should be ``__cpp_lib_is_nothrow_convertible``\ ","Prague","|Complete|","12.0"
diff --git a/libcxx/docs/Status/SpaceshipProjects.csv b/libcxx/docs/Status/SpaceshipProjects.csv
index e1cf2044cfd78..4dc43cdbbd08f 100644
--- a/libcxx/docs/Status/SpaceshipProjects.csv
+++ b/libcxx/docs/Status/SpaceshipProjects.csv
@@ -83,7 +83,7 @@ Section,Description,Dependencies,Assignee,Complete
 "| `[string.view.synop] <https://wg21.link/string.view.synop>`_
 | `[string.view.comparison] <https://wg21.link/string.view.comparison>`_",| `basic_string_view <https://reviews.llvm.org/D130295>`_,None,Mark de Wever,|Complete|
 - `5.7 Clause 22: Containers library <https://wg21.link/p1614r2#clause-22-containers-library>`_,,,,
-| `[container.requirements.general] <https://wg21.link/container.requirements.general>`_,|,None,Unassigned,|Not Started|
+| `[container.requirements.general] <https://wg21.link/container.requirements.general>`_,|,None,Mark de Wever,|Complete|
 | `[array.syn] <https://wg21.link/array.syn>`_ (`general <https://wg21.link/container.opt.reqmts>`_),| `array <https://reviews.llvm.org/D132265>`_,[expos.only.func],"| Adrian Vogelsgesang
 | Hristo Hristov",|Complete|
 | `[deque.syn] <https://wg21.link/deque.syn>`_ (`general <https://wg21.link/container.opt.reqmts>`_),| `deque <https://reviews.llvm.org/D144821>`_,[expos.only.func],Hristo Hristov,|Complete|
diff --git a/libcxx/include/__bit_reference b/libcxx/include/__bit_reference
index 606069d98be72..ba56ac412a100 100644
--- a/libcxx/include/__bit_reference
+++ b/libcxx/include/__bit_reference
@@ -16,6 +16,7 @@
 #include <__bit/countr.h>
 #include <__bit/invert_if.h>
 #include <__bit/popcount.h>
+#include <__compare/strong_order.h>
 #include <__config>
 #include <__fwd/bit_reference.h>
 #include <__iterator/iterator_traits.h>
@@ -913,6 +914,7 @@ public:
     return __x.__seg_ == __y.__seg_ && __x.__ctz_ == __y.__ctz_;
   }
 
+#if _LIBCPP_STD_VER <= 17
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 friend bool
   operator!=(const __bit_iterator& __x, const __bit_iterator& __y) {
     return !(__x == __y);
@@ -937,6 +939,18 @@ public:
   operator>=(const __bit_iterator& __x, const __bit_iterator& __y) {
     return !(__x < __y);
   }
+#else  // _LIBCPP_STD_VER <= 17
+  _LIBCPP_HIDE_FROM_ABI constexpr friend strong_ordering
+  operator<=>(const __bit_iterator& __x, const __bit_iterator& __y) {
+    if (__x.__seg_ < __y.__seg_)
+      return strong_ordering::less;
+
+    if (__x.__seg_ == __y.__seg_)
+      return __x.__ctz_ <=> __y.__ctz_;
+
+    return strong_ordering::greater;
+  }
+#endif // _LIBCPP_STD_VER <= 17
 
 private:
   _LIBCPP_HIDE_FROM_ABI
diff --git a/libcxx/include/__iterator/wrap_iter.h b/libcxx/include/__iterator/wrap_iter.h
index 252d13b26c9e2..a955d3be918b9 100644
--- a/libcxx/include/__iterator/wrap_iter.h
+++ b/libcxx/include/__iterator/wrap_iter.h
@@ -10,6 +10,7 @@
 #ifndef _LIBCPP___ITERATOR_WRAP_ITER_H
 #define _LIBCPP___ITERATOR_WRAP_ITER_H
 
+#include <__compare/strong_order.h>
 #include <__config>
 #include <__iterator/iterator_traits.h>
 #include <__memory/addressof.h>
@@ -119,6 +120,8 @@ operator==(const __wrap_iter<_Iter1>& __x, const __wrap_iter<_Iter2>& __y) _NOEX
   return __x.base() == __y.base();
 }
 
+#if _LIBCPP_STD_VER <= 17
+
 template <class _Iter1>
 _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 bool
 operator<(const __wrap_iter<_Iter1>& __x, const __wrap_iter<_Iter1>& __y) _NOEXCEPT {
@@ -179,6 +182,22 @@ operator<=(const __wrap_iter<_Iter1>& __x, const __wrap_iter<_Iter2>& __y) _NOEX
   return !(__y < __x);
 }
 
+#else // _LIBCPP_STD_VER <= 17
+
+template <class _Iter1>
+_LIBCPP_HIDE_FROM_ABI constexpr strong_ordering
+operator<=>(const __wrap_iter<_Iter1>& __x, const __wrap_iter<_Iter1>& __y) noexcept {
+  return __x.base() <=> __y.base();
+}
+
+template <class _Iter1, class _Iter2>
+_LIBCPP_HIDE_FROM_ABI constexpr strong_ordering
+operator<=>(const __wrap_iter<_Iter1>& __x, const __wrap_iter<_Iter2>& __y) noexcept {
+  return __x.base() <=> __y.base();
+}
+
+#endif // _LIBCPP_STD_VER <= 17
+
 template <class _Iter1, class _Iter2>
 _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14
 #ifndef _LIBCPP_CXX03_LANG
diff --git a/libcxx/include/deque b/libcxx/include/deque
index 4fc994a6e229b..1073187a99909 100644
--- a/libcxx/include/deque
+++ b/libcxx/include/deque
@@ -376,6 +376,7 @@ public:
     return __x.__ptr_ == __y.__ptr_;
   }
 
+#if _LIBCPP_STD_VER <= 17
   _LIBCPP_HIDE_FROM_ABI friend bool operator!=(const __deque_iterator& __x, const __deque_iterator& __y) {
     return !(__x == __y);
   }
@@ -395,6 +396,17 @@ public:
   _LIBCPP_HIDE_FROM_ABI friend bool operator>=(const __deque_iterator& __x, const __deque_iterator& __y) {
     return !(__x < __y);
   }
+#else // _LIBCPP_STD_VER <= 17
+  _LIBCPP_HIDE_FROM_ABI friend strong_ordering operator<=>(const __deque_iterator& __x, const __deque_iterator& __y) {
+    if (__x.__m_iter_ < __y.__m_iter_)
+      return strong_ordering::less;
+
+    if (__x.__m_iter_ == __y.__m_iter_)
+      return __x.__ptr_ <=> __y.__ptr_;
+
+    return strong_ordering::greater;
+  }
+#endif // _LIBCPP_STD_VER <= 17
 
 private:
   _LIBCPP_HIDE_FROM_ABI explicit __deque_iterator(__map_iterator __m, pointer __p) _NOEXCEPT
diff --git a/libcxx/test/std/containers/sequences/array/iterators.pass.cpp b/libcxx/test/std/containers/sequences/array/iterators.pass.cpp
index 106bc45c70998..9cbe3cb396c9a 100644
--- a/libcxx/test/std/containers/sequences/array/iterators.pass.cpp
+++ b/libcxx/test/std/containers/sequences/array/iterators.pass.cpp
@@ -148,6 +148,15 @@ TEST_CONSTEXPR_CXX17 bool tests()
             assert(std::rbegin(c)  != std::rend(c));
             assert(std::cbegin(c)  != std::cend(c));
             assert(std::crbegin(c) != std::crend(c));
+
+#  if TEST_STD_VER >= 20
+            // P1614 + LWG3352
+            std::same_as<std::strong_ordering> decltype(auto) r1 = ii1 <=> ii2;
+            assert(r1 == std::strong_ordering::equal);
+
+            std::same_as<std::strong_ordering> decltype(auto) r2 = ii1 <=> ii2;
+            assert(r2 == std::strong_ordering::equal);
+#  endif
         }
         {
             typedef std::array<int, 0> C;
@@ -189,6 +198,15 @@ TEST_CONSTEXPR_CXX17 bool tests()
             assert(std::rbegin(c)  == std::rend(c));
             assert(std::cbegin(c)  == std::cend(c));
             assert(std::crbegin(c) == std::crend(c));
+
+#  if TEST_STD_VER >= 20
+            // P1614 + LWG3352
+            std::same_as<std::strong_ordering> decltype(auto) r1 = ii1 <=> ii2;
+            assert(r1 == std::strong_ordering::equal);
+
+            std::same_as<std::strong_ordering> decltype(auto) r2 = ii1 <=> ii2;
+            assert(r2 == std::strong_ordering::equal);
+#  endif
         }
     }
 #endif
diff --git a/libcxx/test/std/containers/sequences/deque/iterators.pass.cpp b/libcxx/test/std/containers/sequences/deque/iterators.pass.cpp
index 1f06ffde41ac2..a8b5db656b205 100644
--- a/libcxx/test/std/containers/sequences/deque/iterators.pass.cpp
+++ b/libcxx/test/std/containers/sequences/deque/iterators.pass.cpp
@@ -74,6 +74,15 @@ int main(int, char**)
 //         assert ( cii != c.begin());
 //         assert ( cii != c.cend());
 //         assert ( ii1 != c.end());
+
+#  if TEST_STD_VER >= 20
+        // P1614 + LWG3352
+        std::same_as<std::strong_ordering> decltype(auto) r1 = ii1 <=> ii2;
+        assert(r1 == std::strong_ordering::equal);
+
+        std::same_as<std::strong_ordering> decltype(auto) r2 = ii1 <=> ii2;
+        assert(r2 == std::strong_ordering::equal);
+#  endif // TEST_STD_VER > 20
     }
 #endif
 
diff --git a/libcxx/test/std/containers/sequences/vector.bool/iterators.pass.cpp b/libcxx/test/std/containers/sequences/vector.bool/iterators.pass.cpp
index 9aaaac7a5557f..2a701cc4ad19f 100644
--- a/libcxx/test/std/containers/sequences/vector.bool/iterators.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector.bool/iterators.pass.cpp
@@ -131,6 +131,15 @@ TEST_CONSTEXPR_CXX20 bool tests()
         assert ( (cii >= ii1 ));
         assert (cii - ii1 == 0);
         assert (ii1 - cii == 0);
+
+#  if TEST_STD_VER >= 20
+        // P1614 + LWG3352
+        std::same_as<std::strong_ordering> decltype(auto) r1 = ii1 <=> ii2;
+        assert(r1 == std::strong_ordering::equal);
+
+        std::same_as<std::strong_ordering> decltype(auto) r2 = ii1 <=> ii2;
+        assert(r2 == std::strong_ordering::equal);
+#  endif // TEST_STD_VER > 20
     }
 #endif
 
diff --git a/libcxx/test/std/containers/sequences/vector/iterators.pass.cpp b/libcxx/test/std/containers/sequences/vector/iterators.pass.cpp
index 70e0e35767e09..457791b8ee6af 100644
--- a/libcxx/test/std/containers/sequences/vector/iterators.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector/iterators.pass.cpp
@@ -164,8 +164,16 @@ TEST_CONSTEXPR_CXX20 bool tests()
         assert ( (cii >= ii1 ));
         assert (cii - ii1 == 0);
         assert (ii1 - cii == 0);
+#  if TEST_STD_VER >= 20
+        // P1614 + LWG3352
+        std::same_as<std::strong_ordering> decltype(auto) r1 = ii1 <=> ii2;
+        assert(r1 == std::strong_ordering::equal);
+
+        std::same_as<std::strong_ordering> decltype(auto) r2 = ii1 <=> ii2;
+        assert(r2 == std::strong_ordering::equal);
+#  endif // TEST_STD_VER > 20
     }
-#endif
+#endif // TEST_STD_VER > 11
 
     return true;
 }

github-actions · 2024-07-17T15:50:04Z

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

libcxx/docs/Status/SpaceshipProjects.csv

libcxx/include/__iterator/wrap_iter.h

ldionne

LGTM but we need to update __bounded_iter as mentioned by other reviewers. Thanks!

This implements the requirements for the container iterator requirements for array, deque, vector, and vector<bool>. Implements: - LWG3352 strong_equality isn't a thing Implements parts of: - P1614R2 The Mothership has Landed Fixes: #62486

Note the code is not yet conforming.

ldionne · 2024-07-23T15:46:52Z

libcxx/test/libcxx/iterators/bounded_iter/comparison.pass.cpp

+  tests<three_way_contiguous_iterator<int*>>();
+  static_assert(tests<three_way_contiguous_iterator<int*>>());


I would declare

template <class Iter, bool HasSpaceship = false> TEST_CONSTEXPR_CXX14 bool tests() { ... if constexpr (HasSpaceship) { ... } }

Then:

tests<three_way_contiguous_iterator<int*>, /* has spaceship */ true>(); static_assert(tests<three_way_contiguous_iterator<int*>, /* has spaceship */ true>());

Actually this is no longer needed. This was a left-over while reworking the patch. operator<=> always works on these iterators.

libcxx/test/std/containers/views/views.span/span.iterators/iterator.pass.cpp

ldionne · 2024-07-23T15:50:50Z

libcxx/test/std/containers/views/views.span/span.iterators/iterator.pass.cpp

+  // TODO Test against C++23 after implementing
+  // P2278R4 cbegin should always return a constant iterator


Suggested change

// TODO Test against C++23 after implementing

// P2278R4 cbegin should always return a constant iterator

// TODO Test against C++23 after implementing

// P2278R4 cbegin should always return a constant iterator

// and then remove the #ifdefs

The #ifdef should remain, but guard against C++23.

libcxx/test/std/containers/views/views.span/span.iterators/iterator.pass.cpp

libcxx/include/deque

ldionne · 2024-07-23T15:55:31Z

libcxx/include/__iterator/wrap_iter.h

+template <class _Iter1>
+_LIBCPP_HIDE_FROM_ABI constexpr strong_ordering
+operator<=>(const __wrap_iter<_Iter1>& __x, const __wrap_iter<_Iter1>& __y) noexcept {
+  if constexpr (three_way_comparable<_Iter1, strong_ordering>) {
+    return __x.base() <=> __y.base();
+  } else {
+    if (__x.base() < __y.base())
+      return strong_ordering::less;
+
+    if (__x.base() == __y.base())
+      return strong_ordering::equal;
+
+    return strong_ordering::greater;
+  }
+}


Can't we simply get rid of this entirely? I think the reason we have those for operator< & friends was because we ran into some issue, but I am not certain that issue also exists for operator<=>. I would try removing it and see if that works.

libcxx/include/__iterator/bounded_iter.h

mordante · 2024-07-24T17:43:13Z

/cherry-pick 8d3252a

llvmbot · 2024-07-24T17:47:57Z

Failed to cherry-pick: 8d3252a

https://github.com/llvm/llvm-project/actions/runs/10081464221

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

…lvm#99343) This implements the requirements for the container iterator requirements for array, deque, vector, and `vector<bool>`. Implements: - LWG3352 strong_equality isn't a thing Implements parts of: - P1614R2 The Mothership has Landed Fixes: llvm#62486

This is a followup of llvm#99343. Since that patch was quite late in the LLVM-19 release cycle some of the unneeded relational operator were not removed in C++20. This removes them and gives the change a bit more "backing" time, just in case there are issues with this change in user code. This change should be an NFC.

…99343) Summary: This implements the requirements for the container iterator requirements for array, deque, vector, and `vector<bool>`. Implements: - LWG3352 strong_equality isn't a thing Implements parts of: - P1614R2 The Mothership has Landed Fixes: #62486 Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250694

…lvm#99343) This implements the requirements for the container iterator requirements for array, deque, vector, and `vector<bool>`. Implements: - LWG3352 strong_equality isn't a thing Implements parts of: - P1614R2 The Mothership has Landed Fixes: llvm#62486

This is a followup of #99343. Since that patch was quite late in the LLVM-19 release cycle some of the unneeded relational operator were not removed in C++20. This removes them and gives the change a bit more "baking" time, just in case there are issues with this change in user code. This change is intended to be an NFC.

This is a followup of llvm#99343. Since that patch was quite late in the LLVM-19 release cycle some of the unneeded relational operator were not removed in C++20. This removes them and gives the change a bit more "baking" time, just in case there are issues with this change in user code. This change is intended to be an NFC.

mordante added this to the LLVM 19.X Release milestone Jul 17, 2024

mordante requested a review from a team as a code owner July 17, 2024 15:46

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jul 17, 2024

Zingam reviewed Jul 17, 2024

View reviewed changes

libcxx/docs/Status/SpaceshipProjects.csv Show resolved Hide resolved

mordante force-pushed the users/mordante/spaceship_container_iterators branch 2 times, most recently from d0e8d06 to 00df20a Compare July 17, 2024 17:09

frederick-vs-ja reviewed Jul 18, 2024

View reviewed changes

libcxx/include/__iterator/wrap_iter.h Show resolved Hide resolved

ldionne approved these changes Jul 18, 2024

View reviewed changes

mordante added 2 commits July 20, 2024 14:02

bounded_iterator

2dc8392

mordante force-pushed the users/mordante/spaceship_container_iterators branch 2 times, most recently from 71a6a1b to 2115d64 Compare July 20, 2024 13:09

CI fixes.

cd9e497

mordante force-pushed the users/mordante/spaceship_container_iterators branch 3 times, most recently from c4db2df to b2216ca Compare July 21, 2024 11:02

mordante marked this pull request as draft July 21, 2024 11:03

CI fixes and improvements

0739782

Note the code is not yet conforming.

mordante force-pushed the users/mordante/spaceship_container_iterators branch from b2216ca to 0739782 Compare July 21, 2024 12:30

Polish the patch.

ecd6887

mordante marked this pull request as ready for review July 22, 2024 16:34

ldionne requested changes Jul 23, 2024

View reviewed changes

mordante marked this pull request as draft July 23, 2024 16:56

Addresses review comments.

e94dc78

mordante force-pushed the users/mordante/spaceship_container_iterators branch from 462924b to e94dc78 Compare July 24, 2024 15:43

mordante marked this pull request as ready for review July 24, 2024 15:43

ldionne approved these changes Jul 24, 2024

View reviewed changes

mordante merged commit 8d3252a into main Jul 24, 2024
32 of 34 checks passed

mordante deleted the users/mordante/spaceship_container_iterators branch July 24, 2024 17:42

llvmbot added the release:cherry-pick-failed label Jul 24, 2024

mordante mentioned this pull request Jul 24, 2024

[libc++][spaceship] Implements X::iterator container requirements. (#99343) #100440

Merged

mordante mentioned this pull request Jul 24, 2024

[libc++][spaceship] Removes unneeded relational operators. #100441

Merged

philnik777 mentioned this pull request Aug 2, 2024

[Clang] Add a release note deprecating __is_nullptr #101638

Merged

mgabka mentioned this pull request Aug 22, 2024

Add release note about ABI mgabka/llvm-project#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc++][spaceship] Implements X::iterator container requirements. #99343

[libc++][spaceship] Implements X::iterator container requirements. #99343

mordante commented Jul 17, 2024 •

edited by ldionne

Loading

llvmbot commented Jul 17, 2024

github-actions bot commented Jul 17, 2024 •

edited

Loading

ldionne left a comment

ldionne Jul 23, 2024

mordante Jul 23, 2024

ldionne Jul 23, 2024

mordante Jul 23, 2024

ldionne Jul 23, 2024

mordante commented Jul 24, 2024

llvmbot commented Jul 24, 2024

		tests<three_way_contiguous_iterator<int*>>();
		static_assert(tests<three_way_contiguous_iterator<int*>>());

		// TODO Test against C++23 after implementing
		// P2278R4 cbegin should always return a constant iterator

[libc++][spaceship] Implements X::iterator container requirements. #99343

[libc++][spaceship] Implements X::iterator container requirements. #99343

Conversation

mordante commented Jul 17, 2024 • edited by ldionne Loading

llvmbot commented Jul 17, 2024

github-actions bot commented Jul 17, 2024 • edited Loading

ldionne left a comment

Choose a reason for hiding this comment

ldionne Jul 23, 2024

Choose a reason for hiding this comment

mordante Jul 23, 2024

Choose a reason for hiding this comment

ldionne Jul 23, 2024

Choose a reason for hiding this comment

mordante Jul 23, 2024

Choose a reason for hiding this comment

ldionne Jul 23, 2024

Choose a reason for hiding this comment

mordante commented Jul 24, 2024

llvmbot commented Jul 24, 2024

mordante commented Jul 17, 2024 •

edited by ldionne

Loading

github-actions bot commented Jul 17, 2024 •

edited

Loading