[libcxx-commits] [libcxx] [libc++] Refactor vector<bool> assign functions and add new tests (PR #119502)
via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Dec 11 07:08:42 PST 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-libcxx
Author: Peng Liu (winner245)
<details>
<summary>Changes</summary>
This PR refactors the `assign` functions as well as the copy/move-assignment operators for `vector<bool>`, and augments them with extensive new tests to validate the refactoring.
- `assign(size_type __n, const value_type& __x)`: Instead of using the copy-and-swap idiom inside a `reserve()` call followed by assignments via `std::fill_n` in the previous implementation, this new implementation directly performs an reallocation followed by a `std::fill_n` call. The beneit of this refactoring is two-folds: first, the `reserve()` call has to copy-over all existing elements into the reallocated new space, and then immediately overwrite them by the assignents call to `std::fill_n`, whereas the new implementation only needs to reallocate new space and then do the assignents by `std::fill_n`, without needing to copy-over old elements (which is an O(n) operation). Moreover, the copy-swap idiom used by `reserve` and `assign` requires 2 extra calls to the `swap`, which is completely avoided in this refacotring.
- `__assign_with_size(_ForwardIterator, _Sentinel, difference_type)`: The main change in this refactor is replacing the `__construct_at_end()` call with `std::__copy`. While the `__construct_at_end()` function internally performs some preliminary work and finally calls `std::__copy`, this refactor saves the preliminary work and directly calls `std::__copy`. Moreover, in terms of code clarity, the new implementation of `__assign_with_size` is almost identical to the new implementation of `assign(size_type __n, const value_type& __x)`, with the only difference being `std::__copy` instead of `std::fill_n`. This makes both `assign` functions consistent in terms of implementation.
- Copy-assignment operator: Refactored to call `__assign_with_size` directly, which is exactly equivalent to the previous implementation but avoids code duplication.
- Move-assignment operator: Instead of calling `assign`, it now directly calls the private helper function `__assign_with_size`, saving some indirections.
With the new tests added in this PR for the assignment operators and those in #<!-- -->119163 for the `assign` functions, the validity of the refactored functions are tested in all possible code paths.
---
Full diff: https://github.com/llvm/llvm-project/pull/119502.diff
3 Files Affected:
- (modified) libcxx/include/__vector/vector_bool.h (+9-23)
- (modified) libcxx/test/std/containers/sequences/vector.bool/assign_copy.pass.cpp (+51-32)
- (modified) libcxx/test/std/containers/sequences/vector.bool/assign_move.pass.cpp (+100-63)
``````````diff
diff --git a/libcxx/include/__vector/vector_bool.h b/libcxx/include/__vector/vector_bool.h
index 36eb7f350ac406..a5f0eb0751e0f3 100644
--- a/libcxx/include/__vector/vector_bool.h
+++ b/libcxx/include/__vector/vector_bool.h
@@ -699,14 +699,7 @@ template <class _Allocator>
_LIBCPP_CONSTEXPR_SINCE_CXX20 vector<bool, _Allocator>& vector<bool, _Allocator>::operator=(const vector& __v) {
if (this != std::addressof(__v)) {
__copy_assign_alloc(__v);
- if (__v.__size_) {
- if (__v.__size_ > capacity()) {
- __vdeallocate();
- __vallocate(__v.__size_);
- }
- std::copy(__v.__begin_, __v.__begin_ + __external_cap_to_internal(__v.__size_), __begin_);
- }
- __size_ = __v.__size_;
+ __assign_with_size(__v.begin(), __v.end(), __v.size());
}
return *this;
}
@@ -754,7 +747,7 @@ vector<bool, _Allocator>::operator=(vector&& __v)
template <class _Allocator>
_LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<bool, _Allocator>::__move_assign(vector& __c, false_type) {
if (__alloc_ != __c.__alloc_)
- assign(__c.begin(), __c.end());
+ __assign_with_size(__c.begin(), __c.end(), __c.size());
else
__move_assign(__c, true_type());
}
@@ -773,19 +766,14 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<bool, _Allocator>::__move_assign(vecto
template <class _Allocator>
_LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<bool, _Allocator>::assign(size_type __n, const value_type& __x) {
- __size_ = 0;
if (__n > 0) {
- size_type __c = capacity();
- if (__n <= __c)
- __size_ = __n;
- else {
- vector __v(get_allocator());
- __v.reserve(__recommend(__n));
- __v.__size_ = __n;
- swap(__v);
+ if (__n > capacity()) {
+ __vdeallocate();
+ __vallocate(__n);
}
std::fill_n(begin(), __n, __x);
}
+ this->__size_ = __n;
}
template <class _Allocator>
@@ -814,17 +802,15 @@ template <class _ForwardIterator, class _Sentinel>
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void
vector<bool, _Allocator>::__assign_with_size(_ForwardIterator __first, _Sentinel __last, difference_type __ns) {
_LIBCPP_ASSERT_VALID_INPUT_RANGE(__ns >= 0, "invalid range specified");
-
- clear();
-
- const size_t __n = static_cast<size_type>(__ns);
+ const size_type __n = static_cast<size_type>(__ns);
if (__n) {
if (__n > capacity()) {
__vdeallocate();
__vallocate(__n);
}
- __construct_at_end(__first, __last, __n);
+ std::__copy(__first, __last, this->begin());
}
+ this->__size_ = __n;
}
template <class _Allocator>
diff --git a/libcxx/test/std/containers/sequences/vector.bool/assign_copy.pass.cpp b/libcxx/test/std/containers/sequences/vector.bool/assign_copy.pass.cpp
index c4866ea4c9b45d..889acb88bfcec7 100644
--- a/libcxx/test/std/containers/sequences/vector.bool/assign_copy.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector.bool/assign_copy.pass.cpp
@@ -10,46 +10,65 @@
// vector& operator=(const vector& c);
-#include <vector>
#include <cassert>
-#include "test_macros.h"
-#include "test_allocator.h"
+#include <vector>
+
#include "min_allocator.h"
+#include "test_allocator.h"
+#include "test_macros.h"
-TEST_CONSTEXPR_CXX20 bool tests()
-{
- {
- std::vector<bool, test_allocator<bool> > l(3, true, test_allocator<bool>(5));
- std::vector<bool, test_allocator<bool> > l2(l, test_allocator<bool>(3));
- l2 = l;
- assert(l2 == l);
- assert(l2.get_allocator() == test_allocator<bool>(3));
- }
- {
- std::vector<bool, other_allocator<bool> > l(3, true, other_allocator<bool>(5));
- std::vector<bool, other_allocator<bool> > l2(l, other_allocator<bool>(3));
- l2 = l;
- assert(l2 == l);
- assert(l2.get_allocator() == other_allocator<bool>(5));
- }
+TEST_CONSTEXPR_CXX20 bool tests() {
+ // Test with insufficient space where reallocation occurs during assignment
+ {
+ std::vector<bool, test_allocator<bool> > l(3, true, test_allocator<bool>(5));
+ std::vector<bool, test_allocator<bool> > l2(l, test_allocator<bool>(3));
+ l2 = l;
+ assert(l2 == l);
+ assert(l2.get_allocator() == test_allocator<bool>(3));
+ }
+ {
+ std::vector<bool, other_allocator<bool> > l(3, true, other_allocator<bool>(5));
+ std::vector<bool, other_allocator<bool> > l2(l, other_allocator<bool>(3));
+ l2 = l;
+ assert(l2 == l);
+ assert(l2.get_allocator() == other_allocator<bool>(5));
+ }
#if TEST_STD_VER >= 11
- {
- std::vector<bool, min_allocator<bool> > l(3, true, min_allocator<bool>());
- std::vector<bool, min_allocator<bool> > l2(l, min_allocator<bool>());
- l2 = l;
- assert(l2 == l);
- assert(l2.get_allocator() == min_allocator<bool>());
- }
+ {
+ std::vector<bool, min_allocator<bool> > l(3, true, min_allocator<bool>());
+ std::vector<bool, min_allocator<bool> > l2(l, min_allocator<bool>());
+ l2 = l;
+ assert(l2 == l);
+ assert(l2.get_allocator() == min_allocator<bool>());
+ }
#endif
- return true;
+ // Test with sufficient size where no reallocation occurs during assignment
+ {
+ std::vector<bool, test_allocator<bool> > l(4, true, test_allocator<bool>(5));
+ std::vector<bool, test_allocator<bool> > l2(5, false, test_allocator<bool>(3));
+ l2 = l;
+ assert(l2 == l);
+ assert(l2.get_allocator() == test_allocator<bool>(3));
+ }
+
+ // Test with sufficient spare space where no reallocation occurs during assignment
+ {
+ std::vector<bool, test_allocator<bool> > l(4, true, test_allocator<bool>(5));
+ std::vector<bool, test_allocator<bool> > l2(2, false, test_allocator<bool>(3));
+ l2.reserve(5);
+ l2 = l;
+ assert(l2 == l);
+ assert(l2.get_allocator() == test_allocator<bool>(3));
+ }
+
+ return true;
}
-int main(int, char**)
-{
- tests();
+int main(int, char**) {
+ tests();
#if TEST_STD_VER > 17
- static_assert(tests());
+ static_assert(tests());
#endif
- return 0;
+ return 0;
}
diff --git a/libcxx/test/std/containers/sequences/vector.bool/assign_move.pass.cpp b/libcxx/test/std/containers/sequences/vector.bool/assign_move.pass.cpp
index 10271efc3f4038..e047807d68f735 100644
--- a/libcxx/test/std/containers/sequences/vector.bool/assign_move.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector.bool/assign_move.pass.cpp
@@ -12,79 +12,116 @@
// vector& operator=(vector&& c);
-#include <vector>
#include <cassert>
-#include "test_macros.h"
-#include "test_allocator.h"
+#include <vector>
+
#include "min_allocator.h"
+#include "test_allocator.h"
+#include "test_macros.h"
-TEST_CONSTEXPR_CXX20 bool tests()
-{
- {
- std::vector<bool, test_allocator<bool> > l(test_allocator<bool>(5));
- std::vector<bool, test_allocator<bool> > lo(test_allocator<bool>(5));
- for (int i = 1; i <= 3; ++i)
- {
- l.push_back(i);
- lo.push_back(i);
- }
- std::vector<bool, test_allocator<bool> > l2(test_allocator<bool>(5));
- l2 = std::move(l);
- assert(l2 == lo);
- LIBCPP_ASSERT(l.empty());
- assert(l2.get_allocator() == lo.get_allocator());
+TEST_CONSTEXPR_CXX20 bool tests() {
+ //
+ // Testing for O(1) ownership move
+ //
+ { // Test with pocma = true_type, thus performing an ownership move.
+ std::vector<bool, other_allocator<bool> > l(other_allocator<bool>(5));
+ std::vector<bool, other_allocator<bool> > lo(other_allocator<bool>(5));
+ std::vector<bool, other_allocator<bool> > l2(other_allocator<bool>(6));
+ for (int i = 1; i <= 3; ++i) {
+ l.push_back(i);
+ lo.push_back(i);
+ }
+ l2 = std::move(l);
+ assert(l2 == lo);
+ assert(l.empty());
+ assert(l2.get_allocator() == lo.get_allocator());
+ }
+ { // Test with pocma = false_type and equal allocators, thus performing an ownership move.
+ std::vector<bool, test_allocator<bool> > l(test_allocator<bool>(5));
+ std::vector<bool, test_allocator<bool> > lo(test_allocator<bool>(5));
+ std::vector<bool, test_allocator<bool> > l2(test_allocator<bool>(5));
+ for (int i = 1; i <= 3; ++i) {
+ l.push_back(i);
+ lo.push_back(i);
}
- {
- std::vector<bool, test_allocator<bool> > l(test_allocator<bool>(5));
- std::vector<bool, test_allocator<bool> > lo(test_allocator<bool>(5));
- for (int i = 1; i <= 3; ++i)
- {
- l.push_back(i);
- lo.push_back(i);
- }
- std::vector<bool, test_allocator<bool> > l2(test_allocator<bool>(6));
- l2 = std::move(l);
- assert(l2 == lo);
- assert(!l.empty());
- assert(l2.get_allocator() == test_allocator<bool>(6));
+ l2 = std::move(l);
+ assert(l2 == lo);
+ LIBCPP_ASSERT(l.empty());
+ assert(l2.get_allocator() == lo.get_allocator());
+ }
+ { // Test with pocma = false_type and equal allocators, thus performing an ownership move.
+ std::vector<bool, min_allocator<bool> > l(min_allocator<bool>{});
+ std::vector<bool, min_allocator<bool> > lo(min_allocator<bool>{});
+ std::vector<bool, min_allocator<bool> > l2(min_allocator<bool>{});
+ for (int i = 1; i <= 3; ++i) {
+ l.push_back(i);
+ lo.push_back(i);
}
- {
- std::vector<bool, other_allocator<bool> > l(other_allocator<bool>(5));
- std::vector<bool, other_allocator<bool> > lo(other_allocator<bool>(5));
- for (int i = 1; i <= 3; ++i)
- {
- l.push_back(i);
- lo.push_back(i);
- }
- std::vector<bool, other_allocator<bool> > l2(other_allocator<bool>(6));
- l2 = std::move(l);
- assert(l2 == lo);
- assert(l.empty());
- assert(l2.get_allocator() == lo.get_allocator());
+ l2 = std::move(l);
+ assert(l2 == lo);
+ assert(l.empty());
+ assert(l2.get_allocator() == lo.get_allocator());
+ }
+
+ //
+ // Testing for O(n) element-wise move
+ //
+ { // Test with pocma = false_type and unequal allocators, thus performing an element-wise move.
+ // Reallocation occurs during the element-wise move due to insufficient space.
+ std::vector<bool, test_allocator<bool> > l(test_allocator<bool>(5));
+ std::vector<bool, test_allocator<bool> > lo(test_allocator<bool>(5));
+ std::vector<bool, test_allocator<bool> > l2(test_allocator<bool>(6));
+ for (int i = 1; i <= 3; ++i) {
+ l.push_back(i);
+ lo.push_back(i);
+ }
+ l2 = std::move(l);
+ assert(l2 == lo);
+ assert(!l.empty());
+ assert(l2.get_allocator() == test_allocator<bool>(6));
+ }
+
+ { // Test with pocma = false_type and unequal allocators, thus performing an element-wise move.
+ // No reallocation occurs since source data fits within current size.
+ std::vector<bool, test_allocator<bool> > l(test_allocator<bool>(5));
+ std::vector<bool, test_allocator<bool> > lo(test_allocator<bool>(5));
+ std::vector<bool, test_allocator<bool> > l2(test_allocator<bool>(6));
+ for (int i = 1; i <= 3; ++i) {
+ l.push_back(i);
+ lo.push_back(i);
}
- {
- std::vector<bool, min_allocator<bool> > l(min_allocator<bool>{});
- std::vector<bool, min_allocator<bool> > lo(min_allocator<bool>{});
- for (int i = 1; i <= 3; ++i)
- {
- l.push_back(i);
- lo.push_back(i);
- }
- std::vector<bool, min_allocator<bool> > l2(min_allocator<bool>{});
- l2 = std::move(l);
- assert(l2 == lo);
- assert(l.empty());
- assert(l2.get_allocator() == lo.get_allocator());
+ for (int i = 1; i <= 5; ++i)
+ l2.push_back(i);
+ l2 = std::move(l);
+ assert(l2 == lo);
+ assert(!l.empty());
+ assert(l2.get_allocator() == test_allocator<bool>(6));
+ }
+ { // Test with pocma = false_type and unequal allocators, thus performing an element-wise move.
+ // No reallocation occurs since source data fits within current spare space.
+ std::vector<bool, test_allocator<bool> > l(test_allocator<bool>(5));
+ std::vector<bool, test_allocator<bool> > lo(test_allocator<bool>(5));
+ std::vector<bool, test_allocator<bool> > l2(test_allocator<bool>(6));
+ for (int i = 1; i <= 3; ++i) {
+ l.push_back(i);
+ lo.push_back(i);
}
+ for (int i = 1; i <= 2; ++i)
+ l2.push_back(i);
+ l2.reserve(5);
+ l2 = std::move(l);
+ assert(l2 == lo);
+ assert(!l.empty());
+ assert(l2.get_allocator() == test_allocator<bool>(6));
+ }
- return true;
+ return true;
}
-int main(int, char**)
-{
- tests();
+int main(int, char**) {
+ tests();
#if TEST_STD_VER > 17
- static_assert(tests());
+ static_assert(tests());
#endif
- return 0;
+ return 0;
}
``````````
</details>
https://github.com/llvm/llvm-project/pull/119502
More information about the libcxx-commits
mailing list