Skip to content

[libc++] Accept iterators instead of raw pointers in __uninitialized_allocator_relocate #114552

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

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Nov 1, 2024

This generalizes the algorithm a bit. Unfortunately, we can't make
the call sites cleaner inside std::vector because the arguments being
passed can all be fancy pointers, which may not be contiguous iterators.

…allocator_relocate

This makes the call sites of the function a lot cleaner.
@ldionne ldionne requested a review from a team as a code owner November 1, 2024 15:45
@ldionne
Copy link
Member Author

ldionne commented Nov 1, 2024

CC @philnik777

@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Nov 1, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 1, 2024

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

This makes the call sites of the function a lot cleaner.


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

2 Files Affected:

  • (modified) libcxx/include/__memory/uninitialized_algorithms.h (+16-12)
  • (modified) libcxx/include/__vector/vector.h (+3-6)
diff --git a/libcxx/include/__memory/uninitialized_algorithms.h b/libcxx/include/__memory/uninitialized_algorithms.h
index 3fa948ecc43cff..9abc6bd2bcd2c3 100644
--- a/libcxx/include/__memory/uninitialized_algorithms.h
+++ b/libcxx/include/__memory/uninitialized_algorithms.h
@@ -611,20 +611,22 @@ struct __allocator_has_trivial_destroy<allocator<_Tp>, _Up> : true_type {};
 //                 [__first, __last) doesn't contain any objects
 //
 // The strong exception guarantee is provided if any of the following are true:
-// - is_nothrow_move_constructible<_Tp>
-// - is_copy_constructible<_Tp>
-// - __libcpp_is_trivially_relocatable<_Tp>
-template <class _Alloc, class _Tp>
-_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 void
-__uninitialized_allocator_relocate(_Alloc& __alloc, _Tp* __first, _Tp* __last, _Tp* __result) {
+// - is_nothrow_move_constructible<_ValueType>
+// - is_copy_constructible<_ValueType>
+// - __libcpp_is_trivially_relocatable<_ValueType>
+template <class _Alloc, class _ContiguousIterator>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 void __uninitialized_allocator_relocate(
+    _Alloc& __alloc, _ContiguousIterator __first, _ContiguousIterator __last, _ContiguousIterator __result) {
+  static_assert(__libcpp_is_contiguous_iterator<_ContiguousIterator>::value, "");
+  using _ValueType = typename iterator_traits<_ContiguousIterator>::value_type;
   static_assert(__is_cpp17_move_insertable<_Alloc>::value,
                 "The specified type does not meet the requirements of Cpp17MoveInsertable");
-  if (__libcpp_is_constant_evaluated() || !__libcpp_is_trivially_relocatable<_Tp>::value ||
-      !__allocator_has_trivial_move_construct<_Alloc, _Tp>::value ||
-      !__allocator_has_trivial_destroy<_Alloc, _Tp>::value) {
+  if (__libcpp_is_constant_evaluated() || !__libcpp_is_trivially_relocatable<_ValueType>::value ||
+      !__allocator_has_trivial_move_construct<_Alloc, _ValueType>::value ||
+      !__allocator_has_trivial_destroy<_Alloc, _ValueType>::value) {
     auto __destruct_first = __result;
-    auto __guard =
-        std::__make_exception_guard(_AllocatorDestroyRangeReverse<_Alloc, _Tp*>(__alloc, __destruct_first, __result));
+    auto __guard          = std::__make_exception_guard(
+        _AllocatorDestroyRangeReverse<_Alloc, _ValueType*>(__alloc, __destruct_first, __result));
     auto __iter = __first;
     while (__iter != __last) {
 #if _LIBCPP_HAS_EXCEPTIONS
@@ -639,7 +641,9 @@ __uninitialized_allocator_relocate(_Alloc& __alloc, _Tp* __first, _Tp* __last, _
     std::__allocator_destroy(__alloc, __first, __last);
   } else {
     // Casting to void* to suppress clang complaining that this is technically UB.
-    __builtin_memcpy(static_cast<void*>(__result), __first, sizeof(_Tp) * (__last - __first));
+    __builtin_memcpy(static_cast<void*>(std::__to_address(__result)),
+                     std::__to_address(__first),
+                     sizeof(_ValueType) * (__last - __first));
   }
 }
 
diff --git a/libcxx/include/__vector/vector.h b/libcxx/include/__vector/vector.h
index 844e5d6a210568..c4c37cbe839b73 100644
--- a/libcxx/include/__vector/vector.h
+++ b/libcxx/include/__vector/vector.h
@@ -737,8 +737,7 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void
 vector<_Tp, _Allocator>::__swap_out_circular_buffer(__split_buffer<value_type, allocator_type&>& __v) {
   __annotate_delete();
   auto __new_begin = __v.__begin_ - (__end_ - __begin_);
-  std::__uninitialized_allocator_relocate(
-      __alloc(), std::__to_address(__begin_), std::__to_address(__end_), std::__to_address(__new_begin));
+  std::__uninitialized_allocator_relocate(__alloc(), __begin_, __end_, __new_begin);
   __v.__begin_ = __new_begin;
   __end_       = __begin_; // All the objects have been destroyed by relocating them.
   std::swap(this->__begin_, __v.__begin_);
@@ -760,14 +759,12 @@ vector<_Tp, _Allocator>::__swap_out_circular_buffer(__split_buffer<value_type, a
 
   // Relocate [__p, __end_) first to avoid having a hole in [__begin_, __end_)
   // in case something in [__begin_, __p) throws.
-  std::__uninitialized_allocator_relocate(
-      __alloc(), std::__to_address(__p), std::__to_address(__end_), std::__to_address(__v.__end_));
+  std::__uninitialized_allocator_relocate(__alloc(), __p, __end_, __v.__end_);
   __v.__end_ += (__end_ - __p);
   __end_           = __p; // The objects in [__p, __end_) have been destroyed by relocating them.
   auto __new_begin = __v.__begin_ - (__p - __begin_);
 
-  std::__uninitialized_allocator_relocate(
-      __alloc(), std::__to_address(__begin_), std::__to_address(__p), std::__to_address(__new_begin));
+  std::__uninitialized_allocator_relocate(__alloc(), __begin_, __p, __new_begin);
   __v.__begin_ = __new_begin;
   __end_       = __begin_; // All the objects have been destroyed by relocating them.
 

@ldionne ldionne merged commit 6721bcf into llvm:main Nov 14, 2024
63 checks passed
@ldionne ldionne deleted the review/vector-uninitialized-relocate-with-iterators branch November 14, 2024 10:22
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.

3 participants