[libcxx-commits] [libcxx] [libc++] Fix assignment in insertion into `vector` (PR #116001)
A. Jiang via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Nov 14 00:14:21 PST 2024
https://github.com/frederick-vs-ja updated https://github.com/llvm/llvm-project/pull/116001
>From f89d701a0c4a9b9e2a4b66e80ca599bdb4d5c43d Mon Sep 17 00:00:00 2001
From: "A. Jiang" <de34 at live.cn>
Date: Thu, 14 Nov 2024 16:13:19 +0800
Subject: [PATCH] [libc++] Fix assignment in insertion into `vector`
Changes:
- Avoid direct assignment in iterator-pair `insert` overload and
`insert_range`, except when the assignment is move assignment.
---
libcxx/include/__vector/vector.h | 88 +++++++++++++------
.../insert_iter_iter_iter.pass.cpp | 18 ++++
.../vector.modifiers/insert_range.pass.cpp | 16 ++++
3 files changed, 97 insertions(+), 25 deletions(-)
diff --git a/libcxx/include/__vector/vector.h b/libcxx/include/__vector/vector.h
index 0e1b90e53064b8..cb99d47fbd8108 100644
--- a/libcxx/include/__vector/vector.h
+++ b/libcxx/include/__vector/vector.h
@@ -15,6 +15,7 @@
#include <__algorithm/min.h>
#include <__algorithm/move.h>
#include <__algorithm/move_backward.h>
+#include <__algorithm/ranges_copy_n.h>
#include <__algorithm/rotate.h>
#include <__assert>
#include <__config>
@@ -23,6 +24,7 @@
#include <__fwd/vector.h>
#include <__iterator/advance.h>
#include <__iterator/bounded_iter.h>
+#include <__iterator/concepts.h>
#include <__iterator/distance.h>
#include <__iterator/iterator_traits.h>
#include <__iterator/move_iterator.h>
@@ -54,6 +56,7 @@
#include <__type_traits/is_same.h>
#include <__type_traits/is_trivially_relocatable.h>
#include <__type_traits/type_identity.h>
+#include <__utility/declval.h>
#include <__utility/exception_guard.h>
#include <__utility/forward.h>
#include <__utility/is_pointer_in_range.h>
@@ -570,7 +573,7 @@ class _LIBCPP_TEMPLATE_VIS vector {
if (__n > 0) {
__vallocate(__n);
- __construct_at_end(__first, __last, __n);
+ __construct_at_end(std::move(__first), std::move(__last), __n);
}
__guard.__complete();
@@ -590,9 +593,31 @@ class _LIBCPP_TEMPLATE_VIS vector {
template <class _Iterator, class _Sentinel>
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __assign_with_sentinel(_Iterator __first, _Sentinel __last);
- template <class _ForwardIterator, class _Sentinel>
+ // The `_Iterator` in `*_with_size` functions can be input-only only if called from `*_range` (since C++23).
+ // Otherwise, `_Iterator` is a forward iterator.
+
+ template <class _Iterator, class _Sentinel>
+ _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void
+ __assign_with_size(_Iterator __first, _Sentinel __last, difference_type __n);
+
+ template <class _Iterator,
+ __enable_if_t<!is_same<decltype(*std::declval<_Iterator&>())&&, value_type&&>::value, int> = 0>
+ _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void
+ __insert_assign_n_unchecked(_Iterator __first, difference_type __n, pointer __position) {
+ for (pointer __end_position = __position + __n; __position != __end_position; (void)++__position, ++__first) {
+ __temp_value<value_type, _Allocator> __tmp(this->__alloc(), *__first);
+ *__position = std::move(__tmp.get());
+ }
+ }
+
+ template <class _Iterator,
+ __enable_if_t<is_same<decltype(*std::declval<_Iterator&>())&&, value_type&&>::value, int> = 0>
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void
- __assign_with_size(_ForwardIterator __first, _Sentinel __last, difference_type __n);
+ __insert_assign_n_unchecked(_Iterator __first, difference_type __n, pointer __position) {
+ for (pointer __end_position = __position + __n; __position != __end_position; (void)++__position, ++__first) {
+ *__position = *__first;
+ }
+ }
template <class _InputIterator, class _Sentinel>
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI iterator
@@ -916,7 +941,7 @@ template <class _InputIterator, class _Sentinel>
_LIBCPP_CONSTEXPR_SINCE_CXX20 void
vector<_Tp, _Allocator>::__construct_at_end(_InputIterator __first, _Sentinel __last, size_type __n) {
_ConstructTransaction __tx(*this, __n);
- __tx.__pos_ = std::__uninitialized_allocator_copy(__alloc(), __first, __last, __tx.__pos_);
+ __tx.__pos_ = std::__uninitialized_allocator_copy(__alloc(), std::move(__first), std::move(__last), __tx.__pos_);
}
// Default constructs __n objects starting at __end_
@@ -1023,23 +1048,31 @@ vector<_Tp, _Allocator>::__assign_with_sentinel(_Iterator __first, _Sentinel __l
}
template <class _Tp, class _Allocator>
-template <class _ForwardIterator, class _Sentinel>
+template <class _Iterator, class _Sentinel>
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void
-vector<_Tp, _Allocator>::__assign_with_size(_ForwardIterator __first, _Sentinel __last, difference_type __n) {
+vector<_Tp, _Allocator>::__assign_with_size(_Iterator __first, _Sentinel __last, difference_type __n) {
size_type __new_size = static_cast<size_type>(__n);
if (__new_size <= capacity()) {
if (__new_size > size()) {
- _ForwardIterator __mid = std::next(__first, size());
- std::copy(__first, __mid, this->__begin_);
- __construct_at_end(__mid, __last, __new_size - size());
+#if _LIBCPP_STD_VER >= 23
+ if constexpr (!forward_iterator<_Iterator>) {
+ auto __mid = ranges::copy_n(std::move(__first), size(), this->__begin_).in;
+ __construct_at_end(std::move(__mid), std::move(__last), __new_size - size());
+ } else
+#endif
+ {
+ _Iterator __mid = std::next(__first, size());
+ std::copy(__first, __mid, this->__begin_);
+ __construct_at_end(__mid, __last, __new_size - size());
+ }
} else {
- pointer __m = std::__copy(__first, __last, this->__begin_).second;
+ pointer __m = std::__copy(std::move(__first), __last, this->__begin_).second;
this->__destruct_at_end(__m);
}
} else {
__vdeallocate();
__vallocate(__recommend(__new_size));
- __construct_at_end(__first, __last, __new_size);
+ __construct_at_end(std::move(__first), std::move(__last), __new_size);
}
}
@@ -1297,29 +1330,34 @@ template <class _Iterator, class _Sentinel>
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI typename vector<_Tp, _Allocator>::iterator
vector<_Tp, _Allocator>::__insert_with_size(
const_iterator __position, _Iterator __first, _Sentinel __last, difference_type __n) {
- auto __insertion_size = __n;
- pointer __p = this->__begin_ + (__position - begin());
+ pointer __p = this->__begin_ + (__position - begin());
if (__n > 0) {
if (__n <= this->__end_cap() - this->__end_) {
- size_type __old_n = __n;
pointer __old_last = this->__end_;
- _Iterator __m = std::next(__first, __n);
difference_type __dx = this->__end_ - __p;
if (__n > __dx) {
- __m = __first;
- difference_type __diff = this->__end_ - __p;
- std::advance(__m, __diff);
- __construct_at_end(__m, __last, __n - __diff);
- __n = __dx;
- }
- if (__n > 0) {
- __move_range(__p, __old_last, __p + __old_n);
- std::copy(__first, __m, __p);
+#if _LIBCPP_STD_VER >= 23
+ if constexpr (!forward_iterator<_Iterator>) {
+ __construct_at_end(std::move(__first), std::move(__last), __n);
+ std::rotate(__p, __old_last, this->__end_);
+ } else
+#endif
+ {
+ _Iterator __m = std::next(__first, __dx);
+ __construct_at_end(__m, __last, __n - __dx);
+ if (__dx > 0) {
+ __move_range(__p, __old_last, __p + __n);
+ __insert_assign_n_unchecked(__first, __dx, __p);
+ }
+ }
+ } else {
+ __move_range(__p, __old_last, __p + __n);
+ __insert_assign_n_unchecked(std::move(__first), __n, __p);
}
} else {
allocator_type& __a = this->__alloc();
__split_buffer<value_type, allocator_type&> __v(__recommend(size() + __n), __p - this->__begin_, __a);
- __v.__construct_at_end_with_size(__first, __insertion_size);
+ __v.__construct_at_end_with_size(std::move(__first), __n);
__p = __swap_out_circular_buffer(__v, __p);
}
}
diff --git a/libcxx/test/std/containers/sequences/vector/vector.modifiers/insert_iter_iter_iter.pass.cpp b/libcxx/test/std/containers/sequences/vector/vector.modifiers/insert_iter_iter_iter.pass.cpp
index 934b85ce01c67b..4b7c47cc6388d9 100644
--- a/libcxx/test/std/containers/sequences/vector/vector.modifiers/insert_iter_iter_iter.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector/vector.modifiers/insert_iter_iter_iter.pass.cpp
@@ -138,6 +138,24 @@ TEST_CONSTEXPR_CXX20 bool tests()
for (; j < 105; ++j)
assert(v[j] == 0);
}
+ {
+ struct Wrapper {
+ TEST_CONSTEXPR Wrapper(int n) : n_(n) {}
+
+ int n_;
+
+ private:
+ void operator=(int);
+ };
+
+ int a[] = {1, 2, 3, 4, 5};
+ const std::size_t count = sizeof(a) / sizeof(a[0]);
+ std::vector<Wrapper> v;
+ v.insert(v.end(), a, a + count);
+ assert(v.size() == count);
+ for (std::size_t i = 0; i != count; ++i)
+ assert(v[i].n_ == a[i]);
+ }
#if TEST_STD_VER >= 11
{
typedef std::vector<int, min_allocator<int> > V;
diff --git a/libcxx/test/std/containers/sequences/vector/vector.modifiers/insert_range.pass.cpp b/libcxx/test/std/containers/sequences/vector/vector.modifiers/insert_range.pass.cpp
index 0e26cb1546277b..d56493e0029f25 100644
--- a/libcxx/test/std/containers/sequences/vector/vector.modifiers/insert_range.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector/vector.modifiers/insert_range.pass.cpp
@@ -55,6 +55,22 @@ constexpr bool test() {
}
}
+ { // Ensure that insert_range doesn't use unexpected assignment.
+ struct Wrapper {
+ constexpr Wrapper(int n) : n_(n) {}
+ void operator=(int) = delete;
+
+ int n_;
+ };
+
+ int a[]{1, 2, 3, 4, 5};
+ std::vector<Wrapper> v;
+ v.insert_range(v.end(), a);
+ assert(v.size() == std::size(a));
+ for (std::size_t i = 0; i != std::size(a); ++i)
+ assert(v[i].n_ == a[i]);
+ }
+
return true;
}
More information about the libcxx-commits
mailing list