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

Author: philnik777

libcxx/docs/ReleaseNotes/22.rst

@@ -70,6 +70,7 @@ Improvements and New Features
   in chunks into a buffer.
 - Multiple internal types have been refactored to use ``[[no_unique_address]]``, resulting in faster compile times and
   reduced debug information.
+- The performance of ``deque::append_range`` has been improved by up to 3.4x
 
 - The performance of ``std::find`` has been improved by up to 2x for integral types
 - The ``std::distance`` and ``std::ranges::distance`` algorithms have been optimized for segmented iterators (e.g.,

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 __len = ranges::distance(__range);
+      if (__len < __cap_ - __end_) {
+        __construct_at_end(ranges::begin(__range), ranges::end(__range), __len);
+      } else {
+        __split_buffer<value_type, allocator_type&> __buffer(__recommend(size() + __len), size(), __alloc_);
+        __buffer.__construct_at_end_with_size(ranges::begin(__range), __len);
+        __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
 

libcxx/include/deque

@@ -805,7 +805,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
 
@@ -1459,13 +1463,13 @@ _LIBCPP_HIDE_FROM_ABI void deque<_Tp, _Allocator>::__assign_with_size(_Iterator
 
     auto __i = begin();
     for (auto __count = size(); __count != 0; --__count) {
-      *__i++ = *__f++;
+      *__i++ = *__f;
+      ++__f;
     }
 
-    __append_with_size(__f, __added_size);
-
+    __append_with_size(std::move(__f), __added_size);
   } else {
-    __erase_to_end(std::copy_n(__f, __n, begin()));
+    __erase_to_end(std::copy_n(std::move(__f), __n, begin()));
   }
 }
 

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

@@ -309,7 +309,7 @@ void sequence_container_benchmarks(std::string container) {
   }
 
   /////////////////////////
-  // Variations of push_back
+  // Appending elements
   /////////////////////////
   static constexpr bool has_push_back = requires(Container c, ValueType v) { c.push_back(v); };
   static constexpr bool has_capacity  = requires(Container c) { c.capacity(); };
@@ -399,6 +399,53 @@ void sequence_container_benchmarks(std::string container) {
           st.ResumeTiming();
         }
       });
+
+#if TEST_STD_VER >= 23
+    for (auto gen : generators)
+      bench("append_range() (into empty container)" + tostr(gen), [gen](auto& state) {
+        auto const size = state.range(0);
+        std::vector<ValueType> in;
+        std::generate_n(std::back_inserter(in), size, gen);
+        DoNotOptimizeData(in);
+
+        Container c;
+        DoNotOptimizeData(c);
+        for (auto _ : state) {
+          c.append_range(in);
+          DoNotOptimizeData(c);
+
+          state.PauseTiming();
+          c.clear();
+          state.ResumeTiming();
+        }
+      });
+#endif
+  }
+
+  /////////////////////////
+  // Prepending elements
+  /////////////////////////
+  static constexpr bool has_prepend_range = requires(Container c, std::vector<ValueType> v) { c.prepend_range(v); };
+
+  if constexpr (has_prepend_range) {
+    for (auto gen : generators)
+      bench("prepend_range() (into empty container)" + tostr(gen), [gen](auto& state) {
+        auto const size = state.range(0);
+        std::vector<ValueType> in;
+        std::generate_n(std::back_inserter(in), size, gen);
+        DoNotOptimizeData(in);
+
+        Container c;
+        DoNotOptimizeData(c);
+        for (auto _ : state) {
+          c.prepend_range(in);
+          DoNotOptimizeData(c);
+
+          state.PauseTiming();
+          c.clear();
+          state.ResumeTiming();
+        }
+      });
   }
 
   /////////////////////////

libcxx/test/std/containers/insert_range_helpers.h

@@ -94,6 +94,9 @@ constexpr void for_all_iterators_and_allocators(Func f) {
       f.template operator()<Iter, Iter, safe_allocator<T>>();
     }
   });
+  // This is added because otherwise there would be no input-only sized range, which has fewer guarantees than a
+  // forward and sized range. We don't want to put it in the for_each above to avoid a combinatorial explosion.
+  f.template operator()<cpp20_input_iterator<PtrT>, sized_sentinel<cpp20_input_iterator<PtrT>>, std::allocator<T>>();
 }
 
 // Uses a shorter list of iterator types for use in `constexpr` mode for cases when running the full set in would take

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_emplace_constructible<std::deque>();
 
   test_append_range_exception_safety_throwing_copy<std::deque>();
   test_append_range_exception_safety_throwing_allocator<std::deque, int>();

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 - see https://llvm.org/PR162605
+  // test_sequence_prepend_range_emplace_constructible<std::deque>();
 
   test_prepend_range_exception_safety_throwing_copy<std::deque>();
   test_prepend_range_exception_safety_throwing_allocator<std::deque, int>();

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_emplace_constructible<std::forward_list>();
 
   if (!TEST_IS_CONSTANT_EVALUATED) {
     test_prepend_range_exception_safety_throwing_copy<std::forward_list>();

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

@@ -643,6 +643,61 @@ constexpr void test_sequence_assign_range_move_only() {
   c.assign_range(in);
 }
 
+struct InPlaceOnly {
+  constexpr InPlaceOnly() {}
+  InPlaceOnly(const InPlaceOnly&) = delete;
+  InPlaceOnly(InPlaceOnly&&) = delete;
+  InPlaceOnly& operator=(const InPlaceOnly&) = delete;
+  InPlaceOnly& operator=(InPlaceOnly&&) = delete;
+};
+
+struct EmplaceConstructible {
+  EmplaceConstructible(const EmplaceConstructible&) = delete;
+  EmplaceConstructible& operator=(const EmplaceConstructible&) = delete;
+  EmplaceConstructible& operator=(EmplaceConstructible&&) = delete;
+  EmplaceConstructible(EmplaceConstructible&&) = delete;
+  constexpr EmplaceConstructible(const InPlaceOnly&) {}
+};
+
+template <template <class...> class Container>
+constexpr void test_sequence_append_range_emplace_constructible() {
+  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<EmplaceConstructible> c;
+    c.append_range(in);
+  });
+}
+
+template <template <class...> class Container>
+constexpr void test_sequence_prepend_range_emplace_constructible() {
+  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<EmplaceConstructible> c;
+    c.prepend_range(in);
+  });
+}
+
+// vector has a special requirement that the type also has to be Cpp17MoveInsertable
+struct EmplaceConstructibleAndMoveInsertable {
+  EmplaceConstructibleAndMoveInsertable(const EmplaceConstructibleAndMoveInsertable&) = delete;
+  EmplaceConstructibleAndMoveInsertable& operator=(const EmplaceConstructibleAndMoveInsertable&) = delete;
+  EmplaceConstructibleAndMoveInsertable& operator=(EmplaceConstructibleAndMoveInsertable&&) = delete;
+  constexpr EmplaceConstructibleAndMoveInsertable(EmplaceConstructibleAndMoveInsertable&&) {}
+  constexpr EmplaceConstructibleAndMoveInsertable(const InPlaceOnly&) {}
+};
+
+template <template <class...> class Container>
+constexpr void test_sequence_append_range_emplace_constructible_and_move_insertable() {
+  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<EmplaceConstructibleAndMoveInsertable> c;
+    c.append_range(in);
+  });
+}
+
 // Exception safety.
 
 template <template <class...> class Container>

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_emplace_constructible<std::list>();
 
   if (!TEST_IS_CONSTANT_EVALUATED) {
     test_append_range_exception_safety_throwing_copy<std::list>();