[libc++] Fix {deque,vector}::append_range assuming too much about the types #162438

philnik777 · 2025-10-08T08:21:56Z

Currently, deque and vector's append_range is implemented in terms of insert_range. The problem with that is that insert_range has more preconditions, resulting in us rejecting valid code.

This also significantly improves performance for deque in some cases.

llvmbot · 2025-10-08T08:22:31Z

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

Currently, deque and vector's append_range is implemented in terms of insert_range. The problem with that is that insert_range has more preconditions, resulting in us rejecting valid code.

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

9 Files Affected:

(modified) libcxx/include/__vector/vector.h (+16-1)
(modified) libcxx/include/deque (+5-1)
(modified) libcxx/test/std/containers/sequences/deque/deque.modifiers/append_range.pass.cpp (+1)
(modified) libcxx/test/std/containers/sequences/deque/deque.modifiers/prepend_range.pass.cpp (+2)
(modified) libcxx/test/std/containers/sequences/forwardlist/forwardlist.modifiers/prepend_range.pass.cpp (+1)
(modified) libcxx/test/std/containers/sequences/insert_range_sequence_containers.h (+36)
(modified) libcxx/test/std/containers/sequences/list/list.modifiers/append_range.pass.cpp (+1)
(modified) libcxx/test/std/containers/sequences/list/list.modifiers/prepend_range.pass.cpp (+1)
(modified) libcxx/test/std/containers/sequences/vector/vector.modifiers/append_range.pass.cpp (+1)

diff --git a/libcxx/include/__vector/vector.h b/libcxx/include/__vector/vector.h
index 316d3a9d10eff..247b4c2833e28 100644
--- a/libcxx/include/__vector/vector.h
+++ b/libcxx/include/__vector/vector.h
@@ -42,6 +42,7 @@
 #include <__memory/temp_value.h>
 #include <__memory/uninitialized_algorithms.h>
 #include <__ranges/access.h>
+#include <__ranges/as_rvalue_view.h>
 #include <__ranges/concepts.h>
 #include <__ranges/container_compatible_range.h>
 #include <__ranges/from_range.h>
@@ -489,7 +490,21 @@ class vector {
 #if _LIBCPP_STD_VER >= 23
   template <_ContainerCompatibleRange<_Tp> _Range>
   _LIBCPP_HIDE_FROM_ABI constexpr void append_range(_Range&& __range) {
-    insert_range(end(), std::forward<_Range>(__range));
+    if constexpr (ranges::forward_range<_Range> || ranges::sized_range<_Range>) {
+      auto __size = ranges::distance(__range);
+      if (__size < __cap_ - __end_) {
+        __construct_at_end(ranges::begin(__range), ranges::end(__range), __size);
+      } else {
+        __split_buffer<value_type, allocator_type&> __buffer(__recommend(size() + __size), size(), __alloc_);
+        __buffer.__construct_at_end_with_size(ranges::begin(__range), __size);
+        __swap_out_circular_buffer(__buffer);
+      }
+    } else {
+      vector __buffer(__alloc_);
+      for (auto&& __val : __range)
+        __buffer.emplace_back(std::forward<decltype(__val)>(__val));
+      append_range(ranges::as_rvalue_view(__buffer));
+    }
   }
 #endif
 
diff --git a/libcxx/include/deque b/libcxx/include/deque
index c8e1025eb5dd5..3638abc729091 100644
--- a/libcxx/include/deque
+++ b/libcxx/include/deque
@@ -807,7 +807,11 @@ public:
 
   template <_ContainerCompatibleRange<_Tp> _Range>
   _LIBCPP_HIDE_FROM_ABI void append_range(_Range&& __range) {
-    insert_range(end(), std::forward<_Range>(__range));
+    if constexpr (ranges::forward_range<_Range> || ranges::sized_range<_Range>) {
+      __append_with_size(ranges::begin(__range), ranges::distance(__range));
+    } else {
+      __append_with_sentinel(ranges::begin(__range), ranges::end(__range));
+    }
   }
 #    endif
 
diff --git a/libcxx/test/std/containers/sequences/deque/deque.modifiers/append_range.pass.cpp b/libcxx/test/std/containers/sequences/deque/deque.modifiers/append_range.pass.cpp
index 56a1d226db46f..752b0d898f477 100644
--- a/libcxx/test/std/containers/sequences/deque/deque.modifiers/append_range.pass.cpp
+++ b/libcxx/test/std/containers/sequences/deque/deque.modifiers/append_range.pass.cpp
@@ -31,6 +31,7 @@ int main(int, char**) {
     });
   });
   test_sequence_append_range_move_only<std::deque>();
+  test_sequence_append_range_only_move_construct<std::deque>();
 
   test_append_range_exception_safety_throwing_copy<std::deque>();
   test_append_range_exception_safety_throwing_allocator<std::deque, int>();
diff --git a/libcxx/test/std/containers/sequences/deque/deque.modifiers/prepend_range.pass.cpp b/libcxx/test/std/containers/sequences/deque/deque.modifiers/prepend_range.pass.cpp
index 3154cd389d2f0..2b7d889c8945a 100644
--- a/libcxx/test/std/containers/sequences/deque/deque.modifiers/prepend_range.pass.cpp
+++ b/libcxx/test/std/containers/sequences/deque/deque.modifiers/prepend_range.pass.cpp
@@ -31,6 +31,8 @@ int main(int, char**) {
     });
   });
   test_sequence_prepend_range_move_only<std::deque>();
+  // FIXME: This should work
+  // test_sequence_prepend_range_only_move_construct<std::deque>();
 
   test_prepend_range_exception_safety_throwing_copy<std::deque>();
   test_prepend_range_exception_safety_throwing_allocator<std::deque, int>();
diff --git a/libcxx/test/std/containers/sequences/forwardlist/forwardlist.modifiers/prepend_range.pass.cpp b/libcxx/test/std/containers/sequences/forwardlist/forwardlist.modifiers/prepend_range.pass.cpp
index c4b9cd9bdfc41..3aba10af458cc 100644
--- a/libcxx/test/std/containers/sequences/forwardlist/forwardlist.modifiers/prepend_range.pass.cpp
+++ b/libcxx/test/std/containers/sequences/forwardlist/forwardlist.modifiers/prepend_range.pass.cpp
@@ -30,6 +30,7 @@ TEST_CONSTEXPR_CXX26 bool test() {
     });
   });
   test_sequence_prepend_range_move_only<std::forward_list>();
+  test_sequence_prepend_range_only_move_construct<std::forward_list>();
 
   if (!TEST_IS_CONSTANT_EVALUATED) {
     test_prepend_range_exception_safety_throwing_copy<std::forward_list>();
diff --git a/libcxx/test/std/containers/sequences/insert_range_sequence_containers.h b/libcxx/test/std/containers/sequences/insert_range_sequence_containers.h
index 9f404c46df778..4212c43ed7af9 100644
--- a/libcxx/test/std/containers/sequences/insert_range_sequence_containers.h
+++ b/libcxx/test/std/containers/sequences/insert_range_sequence_containers.h
@@ -643,6 +643,42 @@ constexpr void test_sequence_assign_range_move_only() {
   c.assign_range(in);
 }
 
+struct InPlaceOnly {
+  InPlaceOnly(const InPlaceOnly&) = delete;
+  InPlaceOnly(InPlaceOnly&&) = delete;
+  InPlaceOnly& operator=(const InPlaceOnly&) = delete;
+  InPlaceOnly& operator=(InPlaceOnly&&) = delete;
+  constexpr InPlaceOnly() {}
+};
+
+struct MoveConstructOnly {
+  MoveConstructOnly(const MoveConstructOnly&) = delete;
+  MoveConstructOnly& operator=(const MoveConstructOnly&) = delete;
+  MoveConstructOnly& operator=(MoveConstructOnly&&) = delete;
+  constexpr MoveConstructOnly(MoveConstructOnly&&) {}
+  constexpr MoveConstructOnly(const InPlaceOnly&) {}
+};
+
+template <template <class...> class Container>
+constexpr void test_sequence_append_range_only_move_construct() {
+  InPlaceOnly input[5];
+  types::for_each(types::cpp20_input_iterator_list<InPlaceOnly*>{}, [&]<class Iter> {
+    std::ranges::subrange in(Iter(input), sentinel_wrapper<Iter>(Iter(input + 5)));
+    Container<MoveConstructOnly> c;
+    c.append_range(in);
+  });
+}
+
+template <template <class...> class Container>
+constexpr void test_sequence_prepend_range_only_move_construct() {
+  InPlaceOnly input[5];
+  types::for_each(types::cpp20_input_iterator_list<InPlaceOnly*>{}, [&]<class Iter> {
+    std::ranges::subrange in(Iter(input), sentinel_wrapper<Iter>(Iter(input + 5)));
+    Container<MoveConstructOnly> c;
+    c.prepend_range(in);
+  });
+}
+
 // Exception safety.
 
 template <template <class...> class Container>
diff --git a/libcxx/test/std/containers/sequences/list/list.modifiers/append_range.pass.cpp b/libcxx/test/std/containers/sequences/list/list.modifiers/append_range.pass.cpp
index 4b47a8738e525..8ef3eca86243d 100644
--- a/libcxx/test/std/containers/sequences/list/list.modifiers/append_range.pass.cpp
+++ b/libcxx/test/std/containers/sequences/list/list.modifiers/append_range.pass.cpp
@@ -31,6 +31,7 @@ TEST_CONSTEXPR_CXX26 bool test() {
     });
   });
   test_sequence_append_range_move_only<std::list>();
+  test_sequence_append_range_only_move_construct<std::list>();
 
   if (!TEST_IS_CONSTANT_EVALUATED) {
     test_append_range_exception_safety_throwing_copy<std::list>();
diff --git a/libcxx/test/std/containers/sequences/list/list.modifiers/prepend_range.pass.cpp b/libcxx/test/std/containers/sequences/list/list.modifiers/prepend_range.pass.cpp
index 41f7061c09d28..3d3a25fc448df 100644
--- a/libcxx/test/std/containers/sequences/list/list.modifiers/prepend_range.pass.cpp
+++ b/libcxx/test/std/containers/sequences/list/list.modifiers/prepend_range.pass.cpp
@@ -31,6 +31,7 @@ TEST_CONSTEXPR_CXX26 bool test() {
     });
   });
   test_sequence_prepend_range_move_only<std::list>();
+  test_sequence_prepend_range_only_move_construct<std::list>();
 
   if (!TEST_IS_CONSTANT_EVALUATED) {
     test_prepend_range_exception_safety_throwing_copy<std::list>();
diff --git a/libcxx/test/std/containers/sequences/vector/vector.modifiers/append_range.pass.cpp b/libcxx/test/std/containers/sequences/vector/vector.modifiers/append_range.pass.cpp
index 7a5031e4676f2..c8f2a5ae009e8 100644
--- a/libcxx/test/std/containers/sequences/vector/vector.modifiers/append_range.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector/vector.modifiers/append_range.pass.cpp
@@ -33,6 +33,7 @@ constexpr bool test() {
     });
   });
   test_sequence_append_range_move_only<std::vector>();
+  test_sequence_append_range_only_move_construct<std::vector>();
 
   {   // Vector may or may not need to reallocate because of the insertion -- make sure to test both cases.
     { // Ensure reallocation happens.

github-actions · 2025-10-08T08:25:39Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff origin/main HEAD --extensions ,cpp,h -- libcxx/include/__vector/vector.h libcxx/include/deque libcxx/test/benchmarks/containers/sequence/sequence_container_benchmarks.h libcxx/test/std/containers/insert_range_helpers.h libcxx/test/std/containers/sequences/deque/deque.modifiers/append_range.pass.cpp libcxx/test/std/containers/sequences/deque/deque.modifiers/prepend_range.pass.cpp libcxx/test/std/containers/sequences/forwardlist/forwardlist.modifiers/prepend_range.pass.cpp libcxx/test/std/containers/sequences/insert_range_sequence_containers.h libcxx/test/std/containers/sequences/list/list.modifiers/append_range.pass.cpp libcxx/test/std/containers/sequences/list/list.modifiers/prepend_range.pass.cpp libcxx/test/std/containers/sequences/vector/vector.modifiers/append_range.pass.cpp

⚠️
The reproduction instructions above might return results for more than one PR
in a stack if you are using a stacked PR workflow. You can limit the results by
changing origin/main to the base branch/commit you want to compare against.
⚠️

View the diff from clang-format here.

diff --git a/libcxx/test/std/containers/sequences/insert_range_sequence_containers.h b/libcxx/test/std/containers/sequences/insert_range_sequence_containers.h
index 25096da02..abea4236d 100644
--- a/libcxx/test/std/containers/sequences/insert_range_sequence_containers.h
+++ b/libcxx/test/std/containers/sequences/insert_range_sequence_containers.h
@@ -645,17 +645,17 @@ constexpr void test_sequence_assign_range_move_only() {
 
 struct InPlaceOnly {
   constexpr InPlaceOnly() {}
-  InPlaceOnly(const InPlaceOnly&) = delete;
-  InPlaceOnly(InPlaceOnly&&) = delete;
+  InPlaceOnly(const InPlaceOnly&)            = delete;
+  InPlaceOnly(InPlaceOnly&&)                 = delete;
   InPlaceOnly& operator=(const InPlaceOnly&) = delete;
-  InPlaceOnly& operator=(InPlaceOnly&&) = delete;
+  InPlaceOnly& operator=(InPlaceOnly&&)      = delete;
 };
 
 struct EmplaceConstructible {
-  EmplaceConstructible(const EmplaceConstructible&) = delete;
+  EmplaceConstructible(const EmplaceConstructible&)            = delete;
   EmplaceConstructible& operator=(const EmplaceConstructible&) = delete;
-  EmplaceConstructible& operator=(EmplaceConstructible&&) = delete;
-  EmplaceConstructible(EmplaceConstructible&&) = delete;
+  EmplaceConstructible& operator=(EmplaceConstructible&&)      = delete;
+  EmplaceConstructible(EmplaceConstructible&&)                 = delete;
   constexpr EmplaceConstructible(const InPlaceOnly&) {}
 };
 
@@ -682,9 +682,9 @@ constexpr void test_sequence_prepend_range_emplace_constructible() {
 // vector has a special requirement that the type also has to be Cpp17MoveInsertable
 
 struct EmplaceConstructibleAndMoveInsertable {
-  EmplaceConstructibleAndMoveInsertable(const EmplaceConstructibleAndMoveInsertable&) = delete;
+  EmplaceConstructibleAndMoveInsertable(const EmplaceConstructibleAndMoveInsertable&)            = delete;
   EmplaceConstructibleAndMoveInsertable& operator=(const EmplaceConstructibleAndMoveInsertable&) = delete;
-  EmplaceConstructibleAndMoveInsertable& operator=(EmplaceConstructibleAndMoveInsertable&&) = delete;
+  EmplaceConstructibleAndMoveInsertable& operator=(EmplaceConstructibleAndMoveInsertable&&)      = delete;
   constexpr EmplaceConstructibleAndMoveInsertable(EmplaceConstructibleAndMoveInsertable&&) {}
   constexpr EmplaceConstructibleAndMoveInsertable(const InPlaceOnly&) {}
 };

libcxx/test/std/containers/sequences/insert_range_sequence_containers.h

libcxx/test/std/containers/sequences/deque/deque.modifiers/prepend_range.pass.cpp

libcxx/include/__vector/vector.h

… types

ldionne · 2025-10-09T13:37:54Z

libcxx/test/benchmarks/containers/sequence/sequence_container_benchmarks.h

+
+#if TEST_STD_VER >= 23
+    for (auto gen : generators)
+      bench("append_range()" + tostr(gen), [gen](auto& state) {


Suggested change

bench("append_range()" + tostr(gen), [gen](auto& state) {

bench("append_range() (empty)" + tostr(gen), [gen](auto& state) {

Or do we want to instead benchmark it with some elements already in the sequence?

ldionne · 2025-10-09T13:38:06Z

libcxx/test/benchmarks/containers/sequence/sequence_container_benchmarks.h

+
+  if constexpr (has_prepend_range) {
+    for (auto gen : generators)
+      bench("prepend_range()" + tostr(gen), [gen](auto& state) {


Suggested change

bench("prepend_range()" + tostr(gen), [gen](auto& state) {

bench("prepend_range() (empty)" + tostr(gen), [gen](auto& state) {

Same comment as above.

ldionne · 2025-10-09T13:38:25Z

libcxx/test/std/containers/sequences/insert_range_sequence_containers.h

+}
+
+// vector has a special requirement that the type also has to be Cpp17MoveInsertable
+


Suggested change

ldionne · 2025-10-09T13:40:52Z

libcxx/test/std/containers/insert_range_helpers.h

    f.template operator()<Iter, sentinel_wrapper<Iter>, min_allocator<T>>();
    f.template operator()<Iter, sentinel_wrapper<Iter>, safe_allocator<T>>();

+    f.template operator()<Iter, sized_sentinel<Iter>, std::allocator<T>>();


I wonder if this can have an adverse effect on our tests compile-time. Yes it increases coverage, but the only row in the cross product that we really want to test is cpp20_input_iterator with a sized_sentinel. Maybe we should do that instead (and let's add a comment explaining why we're cherry-picking that row).

philnik777 requested a review from a team as a code owner October 8, 2025 08:21

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Oct 8, 2025

ldionne requested changes Oct 8, 2025

View reviewed changes

[libc++] Fix {deque,vector}::append_range assuming too much about the…

38f2962

… types

philnik777 force-pushed the fix_append_range branch from 1116f84 to 38f2962 Compare October 9, 2025 07:42

ldionne reviewed Oct 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[libc++] Fix {deque,vector}::append_range assuming too much about the types #162438

[libc++] Fix {deque,vector}::append_range assuming too much about the types #162438

philnik777 commented Oct 8, 2025 •

edited

Loading

Uh oh!

llvmbot commented Oct 8, 2025

Uh oh!

github-actions bot commented Oct 8, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ldionne Oct 9, 2025

Uh oh!

ldionne Oct 9, 2025

Uh oh!

ldionne Oct 9, 2025

Uh oh!

ldionne Oct 9, 2025

Uh oh!

Uh oh!

	bench("append_range()" + tostr(gen), [gen](auto& state) {
	bench("append_range() (empty)" + tostr(gen), [gen](auto& state) {

	bench("prepend_range()" + tostr(gen), [gen](auto& state) {
	bench("prepend_range() (empty)" + tostr(gen), [gen](auto& state) {

		}

		// vector has a special requirement that the type also has to be Cpp17MoveInsertable

[libc++] Fix {deque,vector}::append_range assuming too much about the types #162438

Are you sure you want to change the base?

[libc++] Fix {deque,vector}::append_range assuming too much about the types #162438

Conversation

philnik777 commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 8, 2025

Uh oh!

github-actions bot commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ldionne Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

ldionne Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

ldionne Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

ldionne Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

philnik777 commented Oct 8, 2025 •

edited

Loading

github-actions bot commented Oct 8, 2025 •

edited

Loading