Skip to content

[libc++][test] LWG2593: Moved-from state of Allocators #107344

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

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

frederick-vs-ja
Copy link
Contributor

The resolution of LWG2593 didn't require the standard library implementation to change. It merely strengthened requirements on user-defined allocator types and allowed the implementation to make stronger assumptions. The status is tentatively set to Nothing To Do.

However, test_allocator in libc++'s test suit needs to be fixed to conform to the strengthened requirements.

Closes #100220.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner September 5, 2024 02:32
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Sep 5, 2024
@llvmbot
Copy link
Member

llvmbot commented Sep 5, 2024

@llvm/pr-subscribers-libcxx

Author: A. Jiang (frederick-vs-ja)

Changes

The resolution of LWG2593 didn't require the standard library implementation to change. It merely strengthened requirements on user-defined allocator types and allowed the implementation to make stronger assumptions. The status is tentatively set to Nothing To Do.

However, test_allocator in libc++'s test suit needs to be fixed to conform to the strengthened requirements.

Closes #100220.


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

10 Files Affected:

  • (modified) libcxx/docs/Status/Cxx20Issues.csv (+1-1)
  • (modified) libcxx/test/std/containers/associative/map/map.cons/move.pass.cpp (+2-2)
  • (modified) libcxx/test/std/containers/associative/multimap/multimap.cons/move.pass.cpp (+2-2)
  • (modified) libcxx/test/std/containers/associative/multiset/multiset.cons/move.pass.cpp (+2-2)
  • (modified) libcxx/test/std/containers/associative/set/set.cons/move.pass.cpp (+2-2)
  • (modified) libcxx/test/std/containers/container.requirements/container.requirements.general/allocator_move.pass.cpp (+9-8)
  • (modified) libcxx/test/std/containers/sequences/deque/deque.cons/move.pass.cpp (+1-1)
  • (modified) libcxx/test/std/containers/sequences/vector.bool/move.pass.cpp (+9-8)
  • (modified) libcxx/test/std/containers/sequences/vector/vector.cons/move.pass.cpp (+9-8)
  • (modified) libcxx/test/support/test_allocator.h (-1)
diff --git a/libcxx/docs/Status/Cxx20Issues.csv b/libcxx/docs/Status/Cxx20Issues.csv
index e5d2498473ecde..c79289968811bd 100644
--- a/libcxx/docs/Status/Cxx20Issues.csv
+++ b/libcxx/docs/Status/Cxx20Issues.csv
@@ -1,7 +1,7 @@
 "Issue #","Issue Name","Meeting","Status","First released version","Notes"
 "`LWG2070 <https://wg21.link/LWG2070>`__","``allocate_shared``\  should use ``allocator_traits<A>::construct``\ ","2017-07 (Toronto)","|Nothing To Do|","","Resolved by `P0674R1 <https://wg21.link/P0674R1>`__"
 "`LWG2444 <https://wg21.link/LWG2444>`__","Inconsistent complexity for ``std::sort_heap``\ ","2017-07 (Toronto)","|Nothing To Do|","",""
-"`LWG2593 <https://wg21.link/LWG2593>`__","Moved-from state of Allocators","2017-07 (Toronto)","","",""
+"`LWG2593 <https://wg21.link/LWG2593>`__","Moved-from state of Allocators","2017-07 (Toronto)","|Nothing To Do|","",""
 "`LWG2597 <https://wg21.link/LWG2597>`__","``std::log``\  misspecified for complex numbers","2017-07 (Toronto)","","",""
 "`LWG2783 <https://wg21.link/LWG2783>`__","``stack::emplace()``\  and ``queue::emplace()``\  should return ``decltype(auto)``\ ","2017-07 (Toronto)","|Complete|","",""
 "`LWG2932 <https://wg21.link/LWG2932>`__","Constraints on parallel algorithm implementations are underspecified","2017-07 (Toronto)","","",""
diff --git a/libcxx/test/std/containers/associative/map/map.cons/move.pass.cpp b/libcxx/test/std/containers/associative/map/map.cons/move.pass.cpp
index 0e68ba4a17fe6e..0afe64a93d7bdd 100644
--- a/libcxx/test/std/containers/associative/map/map.cons/move.pass.cpp
+++ b/libcxx/test/std/containers/associative/map/map.cons/move.pass.cpp
@@ -35,7 +35,7 @@ int main(int, char**)
         assert(m.size() == 0);
         assert(std::distance(m.begin(), m.end()) == 0);
 
-        assert(mo.get_allocator() == A(test_alloc_base::moved_value));
+        assert(mo.get_allocator() == A(7));
         assert(mo.key_comp() == C(5));
         assert(mo.size() == 0);
         assert(std::distance(mo.begin(), mo.end()) == 0);
@@ -65,7 +65,7 @@ int main(int, char**)
         assert(*std::next(m.begin()) == V(2, 1));
         assert(*std::next(m.begin(), 2) == V(3, 1));
 
-        assert(mo.get_allocator() == A(test_alloc_base::moved_value));
+        assert(mo.get_allocator() == A(7));
         assert(mo.key_comp() == C(5));
         assert(mo.size() == 0);
         assert(std::distance(mo.begin(), mo.end()) == 0);
diff --git a/libcxx/test/std/containers/associative/multimap/multimap.cons/move.pass.cpp b/libcxx/test/std/containers/associative/multimap/multimap.cons/move.pass.cpp
index f4170fb56229d9..6458231e9a4d3c 100644
--- a/libcxx/test/std/containers/associative/multimap/multimap.cons/move.pass.cpp
+++ b/libcxx/test/std/containers/associative/multimap/multimap.cons/move.pass.cpp
@@ -35,7 +35,7 @@ int main(int, char**)
         assert(m.size() == 0);
         assert(std::distance(m.begin(), m.end()) == 0);
 
-        assert(mo.get_allocator() == A(test_alloc_base::moved_value));
+        assert(mo.get_allocator() == A(7));
         assert(mo.key_comp() == C(5));
         assert(mo.size() == 0);
         assert(std::distance(mo.begin(), mo.end()) == 0);
@@ -71,7 +71,7 @@ int main(int, char**)
         assert(*std::next(m.begin(), 7) == V(3, 1.5));
         assert(*std::next(m.begin(), 8) == V(3, 2));
 
-        assert(mo.get_allocator() == A(test_alloc_base::moved_value));
+        assert(mo.get_allocator() == A(7));
         assert(mo.key_comp() == C(5));
         assert(mo.size() == 0);
         assert(std::distance(mo.begin(), mo.end()) == 0);
diff --git a/libcxx/test/std/containers/associative/multiset/multiset.cons/move.pass.cpp b/libcxx/test/std/containers/associative/multiset/multiset.cons/move.pass.cpp
index 9b265b3539afd6..65d297d3bfd45d 100644
--- a/libcxx/test/std/containers/associative/multiset/multiset.cons/move.pass.cpp
+++ b/libcxx/test/std/containers/associative/multiset/multiset.cons/move.pass.cpp
@@ -35,7 +35,7 @@ int main(int, char**)
         assert(m.size() == 0);
         assert(std::distance(m.begin(), m.end()) == 0);
 
-        assert(mo.get_allocator() == A(test_alloc_base::moved_value));
+        assert(mo.get_allocator() == A(7));
         assert(mo.key_comp() == C(5));
         assert(mo.size() == 0);
         assert(std::distance(mo.begin(), mo.end()) == 0);
@@ -72,7 +72,7 @@ int main(int, char**)
         assert(*std::next(m.begin(), 7) == 3);
         assert(*std::next(m.begin(), 8) == 3);
 
-        assert(mo.get_allocator() == A(test_alloc_base::moved_value));
+        assert(mo.get_allocator() == A(7));
         assert(mo.key_comp() == C(5));
         assert(mo.size() == 0);
         assert(std::distance(mo.begin(), mo.end()) == 0);
diff --git a/libcxx/test/std/containers/associative/set/set.cons/move.pass.cpp b/libcxx/test/std/containers/associative/set/set.cons/move.pass.cpp
index 7fa0f8da4d623d..3824ea2d322b8a 100644
--- a/libcxx/test/std/containers/associative/set/set.cons/move.pass.cpp
+++ b/libcxx/test/std/containers/associative/set/set.cons/move.pass.cpp
@@ -35,7 +35,7 @@ int main(int, char**)
         assert(m.size() == 0);
         assert(std::distance(m.begin(), m.end()) == 0);
 
-        assert(mo.get_allocator() == A(test_alloc_base::moved_value));
+        assert(mo.get_allocator() == A(7));
         assert(mo.key_comp() == C(5));
         assert(mo.size() == 0);
         assert(std::distance(mo.begin(), mo.end()) == 0);
@@ -66,7 +66,7 @@ int main(int, char**)
         assert(*std::next(m.begin()) == 2);
         assert(*std::next(m.begin(), 2) == 3);
 
-        assert(mo.get_allocator() == A(test_alloc_base::moved_value));
+        assert(mo.get_allocator() == A(7));
         assert(mo.key_comp() == C(5));
         assert(mo.size() == 0);
         assert(std::distance(mo.begin(), mo.end()) == 0);
diff --git a/libcxx/test/std/containers/container.requirements/container.requirements.general/allocator_move.pass.cpp b/libcxx/test/std/containers/container.requirements/container.requirements.general/allocator_move.pass.cpp
index cc8949e751dd0e..5f2f3cfe16588d 100644
--- a/libcxx/test/std/containers/container.requirements/container.requirements.general/allocator_move.pass.cpp
+++ b/libcxx/test/std/containers/container.requirements/container.requirements.general/allocator_move.pass.cpp
@@ -50,14 +50,15 @@ void test(int expected_num_allocs = 1) {
     assert(alloc_stats.copied == 0);
     assert(alloc_stats.moved == num_stored_allocs);
     {
-      const AllocT& a = v.get_allocator();
-      assert(a.get_id() == test_alloc_base::moved_value);
-      assert(a.get_data() == test_alloc_base::moved_value);
-    }
-    {
-      const AllocT& a = v2.get_allocator();
-      assert(a.get_id() == 101);
-      assert(a.get_data() == 42);
+      const AllocT& a1 = v.get_allocator();
+      assert(a1.get_id() == test_alloc_base::moved_value);
+      assert(a1.get_data() == 42);
+
+      const AllocT& a2 = v2.get_allocator();
+      assert(a2.get_id() == 101);
+      assert(a2.get_data() == 42);
+
+      assert(a1 == a2);
     }
   }
 }
diff --git a/libcxx/test/std/containers/sequences/deque/deque.cons/move.pass.cpp b/libcxx/test/std/containers/sequences/deque/deque.cons/move.pass.cpp
index 067b8eb1667bc5..daed255a5f3a75 100644
--- a/libcxx/test/std/containers/sequences/deque/deque.cons/move.pass.cpp
+++ b/libcxx/test/std/containers/sequences/deque/deque.cons/move.pass.cpp
@@ -38,7 +38,7 @@ int main(int, char**)
         assert(c2 == c3);
         assert(c1.size() == 0);
         assert(c3.get_allocator() == old_a);
-        assert(c1.get_allocator() == A(test_alloc_base::moved_value));
+        assert(c1.get_allocator() == A(1));
         LIBCPP_ASSERT(is_double_ended_contiguous_container_asan_correct(c1));
         LIBCPP_ASSERT(is_double_ended_contiguous_container_asan_correct(c2));
         LIBCPP_ASSERT(is_double_ended_contiguous_container_asan_correct(c3));
diff --git a/libcxx/test/std/containers/sequences/vector.bool/move.pass.cpp b/libcxx/test/std/containers/sequences/vector.bool/move.pass.cpp
index 0e65d9896f4020..0f0107ed0809f6 100644
--- a/libcxx/test/std/containers/sequences/vector.bool/move.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector.bool/move.pass.cpp
@@ -80,14 +80,15 @@ TEST_CONSTEXPR_CXX20 bool tests()
       assert(alloc_stats.copied == 0);
       assert(alloc_stats.moved == 1);
       {
-        const AllocT& a = v.get_allocator();
-        assert(a.get_id() == test_alloc_base::moved_value);
-        assert(a.get_data() == test_alloc_base::moved_value);
-      }
-      {
-        const AllocT& a = v2.get_allocator();
-        assert(a.get_id() == 101);
-        assert(a.get_data() == 42);
+        const AllocT& a1 = v.get_allocator();
+        assert(a1.get_id() == test_alloc_base::moved_value);
+        assert(a2.get_data() == 42);
+
+        const AllocT& a2 = v2.get_allocator();
+        assert(a2.get_id() == 101);
+        assert(a2.get_data() == 42);
+
+        assert(a1 == a2);
       }
     }
 
diff --git a/libcxx/test/std/containers/sequences/vector/vector.cons/move.pass.cpp b/libcxx/test/std/containers/sequences/vector/vector.cons/move.pass.cpp
index 95b896b41f530f..c8b8701cc491b7 100644
--- a/libcxx/test/std/containers/sequences/vector/vector.cons/move.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector/vector.cons/move.pass.cpp
@@ -148,14 +148,15 @@ TEST_CONSTEXPR_CXX20 bool tests()
       assert(alloc_stats.copied == 0);
       assert(alloc_stats.moved == 1);
       {
-        const test_allocator<int>& a = v.get_allocator();
-        assert(a.get_id() == test_alloc_base::moved_value);
-        assert(a.get_data() == test_alloc_base::moved_value);
-      }
-      {
-        const test_allocator<int>& a = v2.get_allocator();
-        assert(a.get_id() == 101);
-        assert(a.get_data() == 42);
+        const test_allocator<int>& a1 = v.get_allocator();
+        assert(a1.get_id() == test_alloc_base::moved_value);
+        assert(a1.get_data() == 42);
+
+        const test_allocator<int>& a2 = v2.get_allocator();
+        assert(a2.get_id() == 101);
+        assert(a2.get_data() == 42);
+
+        assert(a1 == a2);
       }
     }
 
diff --git a/libcxx/test/support/test_allocator.h b/libcxx/test/support/test_allocator.h
index 8ae04413f8a93c..dcd15332ca304f 100644
--- a/libcxx/test/support/test_allocator.h
+++ b/libcxx/test/support/test_allocator.h
@@ -125,7 +125,6 @@ class test_allocator {
     }
     assert(a.data_ != test_alloc_base::destructed_value && a.id_ != test_alloc_base::destructed_value &&
            "moving from destroyed allocator");
-    a.data_ = test_alloc_base::moved_value;
     a.id_ = test_alloc_base::moved_value;
   }
 

The resolution of LWG2593 didn't require the standard library
implementation to change. It merely strengthened requirements on
user-defined allocator types and allowed the implementation to make
stronger assumptions. The status is tentatively set to Nothing To Do.

However, `test_allocator` in libc++'s test suit needs to be fixed to
conform to the strengthened requirements.
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, thanks!

@ldionne ldionne merged commit 46a76c3 into llvm:main Sep 10, 2024
65 checks passed
@frederick-vs-ja frederick-vs-ja deleted the lwg-2593 branch September 10, 2024 13:56
frederick-vs-ja added a commit that referenced this pull request Sep 30, 2024
Previous PR #107344 fixed move constructor of `test_allocator` but
dropped test coverage for move construction in some cases. This PR
attempts to restore the test coverage.

Thanks @Quuxplusone for reminding.
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.

LWG2593: Moved-from state of Allocators
3 participants