[libc++] Avoid discarding const-ness when copy-constructing vectors

[ldionne]

Copy-constructing a std::vector currently discards the const-ness of the elements of the source vector. So for example, if the elements in the vector have both T(T&) and T(T const&) constructors, the non-const constructor is going to be preferred. This patch fixes that and makes sure that constness is respected by the copy constructor and copy assignment operators.

rdar://107652763

[philnik777]

Do you have a link to the standardese that requires us to copy-construct from a const object?

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

Copy-constructing a std::vector<T> currently discards the const-ness of the elements of the source vector. So for example, if the elements in the vector have both T(T&) and T(T const&) constructors, the non-const constructor is going to be preferred. This patch fixes that and makes sure that constness is respected by the copy constructor and copy assignment operators.

rdar://107652763

---

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

4 Files Affected:

• (modified) libcxx/include/vector (+3-3)
• (modified) libcxx/test/std/containers/sequences/vector/vector.cons/assign_copy.pass.cpp (+20)
• (modified) libcxx/test/std/containers/sequences/vector/vector.cons/copy.pass.cpp (+12)
• (modified) libcxx/test/std/containers/sequences/vector/vector.cons/copy_alloc.pass.cpp (+13)

diff --git a/libcxx/include/vector b/libcxx/include/vector
index 4ec6b602371eae..d515dbbcf45088 100644
--- a/libcxx/include/vector
+++ b/libcxx/include/vector
@@ -1276,7 +1276,7 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20
 vector<_Tp, _Allocator>::vector(const vector& __x)
     : __end_cap_(nullptr, __alloc_traits::select_on_container_copy_construction(__x.__alloc()))
 {
-  __init_with_size(__x.__begin_, __x.__end_, __x.size());
+  __init_with_size(__x.begin(), __x.end(), __x.size());
 }
 
 template <class _Tp, class _Allocator>
@@ -1284,7 +1284,7 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20
 vector<_Tp, _Allocator>::vector(const vector& __x, const __type_identity_t<allocator_type>& __a)
     : __end_cap_(nullptr, __a)
 {
-  __init_with_size(__x.__begin_, __x.__end_, __x.size());
+  __init_with_size(__x.begin(), __x.end(), __x.size());
 }
 
 template <class _Tp, class _Allocator>
@@ -1409,7 +1409,7 @@ vector<_Tp, _Allocator>::operator=(const vector& __x)
     if (this != std::addressof(__x))
     {
         __copy_assign_alloc(__x);
-        assign(__x.__begin_, __x.__end_);
+        assign(__x.begin(), __x.end());
     }
     return *this;
 }
diff --git a/libcxx/test/std/containers/sequences/vector/vector.cons/assign_copy.pass.cpp b/libcxx/test/std/containers/sequences/vector/vector.cons/assign_copy.pass.cpp
index e0e227e8dc0c64..98b166d51ab06f 100644
--- a/libcxx/test/std/containers/sequences/vector/vector.cons/assign_copy.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector/vector.cons/assign_copy.pass.cpp
@@ -17,6 +17,19 @@
 #include "min_allocator.h"
 #include "allocators.h"
 
+struct HasNonConstCopyAssignment {
+    HasNonConstCopyAssignment() = default;
+    HasNonConstCopyAssignment(HasNonConstCopyAssignment const&) = default;
+    HasNonConstCopyAssignment(HasNonConstCopyAssignment&) { assert(false); }
+    TEST_CONSTEXPR HasNonConstCopyAssignment& operator=(HasNonConstCopyAssignment const&) {
+        return *this;
+    }
+    HasNonConstCopyAssignment& operator=(HasNonConstCopyAssignment&) {
+        assert(false);
+        return *this;
+    }
+};
+
 TEST_CONSTEXPR_CXX20 bool tests() {
     {
         std::vector<int, test_allocator<int> > l(3, 2, test_allocator<int>(5));
@@ -32,6 +45,13 @@ TEST_CONSTEXPR_CXX20 bool tests() {
         assert(l2 == l);
         assert(l2.get_allocator() == other_allocator<int>(5));
     }
+    {
+        // Make sure we copy elements of the vector using the right copy assignment operator.
+        std::vector<HasNonConstCopyAssignment> v(5);
+        std::vector<HasNonConstCopyAssignment> v2;
+        v2 = v;
+        assert(v2.size() == 5);
+    }
 #if TEST_STD_VER >= 11
     {
         // Test with Allocator::propagate_on_container_copy_assignment == false_type
diff --git a/libcxx/test/std/containers/sequences/vector/vector.cons/copy.pass.cpp b/libcxx/test/std/containers/sequences/vector/vector.cons/copy.pass.cpp
index b0e626381454e0..370e7abc6746aa 100644
--- a/libcxx/test/std/containers/sequences/vector/vector.cons/copy.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector/vector.cons/copy.pass.cpp
@@ -18,6 +18,12 @@
 #include "min_allocator.h"
 #include "asan_testing.h"
 
+struct HasNonConstCopyConstructor {
+    HasNonConstCopyConstructor() = default;
+    TEST_CONSTEXPR HasNonConstCopyConstructor(HasNonConstCopyConstructor const&) { }
+    HasNonConstCopyConstructor(HasNonConstCopyConstructor&) { assert(false); }
+};
+
 template <class C>
 TEST_CONSTEXPR_CXX20 void
 test(const C& x)
@@ -58,6 +64,12 @@ TEST_CONSTEXPR_CXX20 bool tests() {
         assert(is_contiguous_container_asan_correct(v2));
         assert(v2.empty());
     }
+    {
+        // Make sure we copy elements of the vector using the right copy constructor.
+        std::vector<HasNonConstCopyConstructor> v(5);
+        std::vector<HasNonConstCopyConstructor> v2 = v;
+        assert(v2.size() == 5);
+    }
 #if TEST_STD_VER >= 11
     {
         std::vector<int, other_allocator<int> > v(3, 2, other_allocator<int>(5));
diff --git a/libcxx/test/std/containers/sequences/vector/vector.cons/copy_alloc.pass.cpp b/libcxx/test/std/containers/sequences/vector/vector.cons/copy_alloc.pass.cpp
index 0c03c50aed2e8b..8a1b599a1a656d 100644
--- a/libcxx/test/std/containers/sequences/vector/vector.cons/copy_alloc.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector/vector.cons/copy_alloc.pass.cpp
@@ -18,6 +18,12 @@
 #include "min_allocator.h"
 #include "asan_testing.h"
 
+struct HasNonConstCopyConstructor {
+    HasNonConstCopyConstructor() = default;
+    TEST_CONSTEXPR HasNonConstCopyConstructor(HasNonConstCopyConstructor const&) { }
+    HasNonConstCopyConstructor(HasNonConstCopyConstructor&) { assert(false); }
+};
+
 template <class C>
 TEST_CONSTEXPR_CXX20 void
 test(const C& x, const typename C::allocator_type& a)
@@ -56,6 +62,13 @@ TEST_CONSTEXPR_CXX20 bool tests() {
         assert(l2.get_allocator() == other_allocator<int>(3));
         assert(l2.empty());
     }
+    {
+        // Make sure we copy elements of the vector using the right copy constructor.
+        using Alloc = other_allocator<HasNonConstCopyConstructor>;
+        std::vector<HasNonConstCopyConstructor, Alloc> v(5);
+        std::vector<HasNonConstCopyConstructor, Alloc> v2(v, Alloc(42));
+        assert(v2.size() == 5);
+    }
 #if TEST_STD_VER >= 11
     {
         int a[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 8, 7, 6, 5, 4, 3, 1, 0};

[EricWF]

Do you have a link to the standardese that requires us to copy-construct from a const object?

We're given the vector with a const qualifier, we should propagate that const qualifier.

@ldionne I believe we now pass wrapped iterators rather than raw pointers through to the underlying copy mechanism.
Could you ensure that this doesn't cause performance regressions?

[philnik777]

This is actually a precondition violation, since https://eel.is/c++draft/container.requirements#container.reqmts-12 says T has to be Cpp17CopyInsertable, which in turn says allocator_traits<A>::construct(m, p, v) is valid and "The value of v is unchanged and is equivalent to *p.". "v denotes an lvalue of type (possibly const) X or an rvalue of type const X," according to https://eel.is/c++draft/container.requirements#container.intro.reqmts-1.6.