Fix small_vector insert for non-default constructible types

Summary:
Instead of default constructing in `moveObjectsRight`, insert the objects
directly into the hole. This has the added benefit of avoid a default
construction. The creation function has to be careful to return references
(rvalue, or lvalue) to avoid extra constructions without guaranteed NRVO.

I've constructed the elements in reverse order since it is strange to copy
backwards, then construct forwards, but don't have strong opinions, since
either way is correct.

Reviewed By: ot

Differential Revision: D14883034

fbshipit-source-id: bc0d19bf94705718500b80e2d6c202784134e7a5

folly/small_vector.h

@@ -98,20 +98,27 @@ namespace detail {
 
 /*
  * Move objects in memory to the right into some uninitialized
- * memory, where the region overlaps.  This doesn't just use
- * std::move_backward because move_backward only works if all the
- * memory is initialized to type T already.
+ * memory, where the region overlaps. Then call create(size_t) for each
+ * "hole" passing it the offset of the hole from first.
+ *
+ * This doesn't just use std::move_backward because move_backward only works
+ * if all the memory is initialized to type T already.
+ *
+ * The create function should return a reference type, to avoid
+ * extra copies and moves for non-trivial types.
  */
-template <class T>
-typename std::enable_if<
-    std::is_default_constructible<T>::value &&
-    !folly::is_trivially_copyable<T>::value>::type
-moveObjectsRight(T* first, T* lastConstructed, T* realLast) {
+template <class T, class Create>
+typename std::enable_if<!folly::is_trivially_copyable<T>::value>::type
+moveObjectsRightAndCreate(
+    T* const first,
+    T* const lastConstructed,
+    T* const realLast,
+    Create&& create) {
   if (lastConstructed == realLast) {
     return;
   }
 
-  T* end = first - 1; // Past the end going backwards.
+  T* const end = first - 1; // Past the end going backwards.
   T* out = realLast - 1;
   T* in = lastConstructed - 1;
   {
@@ -133,21 +140,33 @@ moveObjectsRight(T* first, T* lastConstructed, T* realLast) {
       *out = std::move(*in);
     }
     for (; out >= lastConstructed; --out) {
-      new (out) T();
+      new (out) T(create(out - first));
+    }
+    for (; out != end; --out) {
+      *out = create(out - first);
     }
     rollback.dismiss();
   }
 }
 
 // Specialization for trivially copyable types.  The call to
-// std::move_backward here will just turn into a memmove.  (TODO:
-// change to std::is_trivially_copyable when that works.)
-template <class T>
-typename std::enable_if<
-    !std::is_default_constructible<T>::value ||
-    folly::is_trivially_copyable<T>::value>::type
-moveObjectsRight(T* first, T* lastConstructed, T* realLast) {
+// std::move_backward here will just turn into a memmove.
+// This must only be used with trivially copyable types because some of the
+// memory may be uninitialized, and std::move_backward() won't work when it
+// can't memmove().
+template <class T, class Create>
+typename std::enable_if<folly::is_trivially_copyable<T>::value>::type
+moveObjectsRightAndCreate(
+    T* const first,
+    T* const lastConstructed,
+    T* const realLast,
+    Create&& create) {
   std::move_backward(first, lastConstructed, realLast);
+  T* const end = first - 1;
+  T* out = first + (realLast - lastConstructed) - 1;
+  for (; out != end; --out) {
+    *out = create(out - first);
+  }
 }
 
 /*
@@ -795,10 +814,16 @@ class small_vector : public detail::small_vector_base<
           offset);
       this->setSize(this->size() + 1);
     } else {
-      detail::moveObjectsRight(
-          data() + offset, data() + size(), data() + size() + 1);
+      detail::moveObjectsRightAndCreate(
+          data() + offset,
+          data() + size(),
+          data() + size() + 1,
+          [&](size_t i) -> value_type&& {
+            assert(i == 0);
+            (void)i;
+            return std::move(t);
+          });
       this->setSize(size() + 1);
-      data()[offset] = std::move(t);
     }
     return begin() + offset;
   }
@@ -812,10 +837,16 @@ class small_vector : public detail::small_vector_base<
   iterator insert(const_iterator pos, size_type n, value_type const& val) {
     auto offset = pos - begin();
     makeSize(size() + n);
-    detail::moveObjectsRight(
-        data() + offset, data() + size(), data() + size() + n);
+    detail::moveObjectsRightAndCreate(
+        data() + offset,
+        data() + size(),
+        data() + size() + n,
+        [&](size_t i) -> value_type const& {
+          assert(i < n);
+          (void)i;
+          return val;
+        });
     this->setSize(size() + n);
-    std::generate_n(begin() + offset, n, [&] { return val; });
     return begin() + offset;
   }
 
@@ -919,7 +950,8 @@ class small_vector : public detail::small_vector_base<
   // iterator insert functions from integral types (see insert().)
   template <class It>
   iterator insertImpl(iterator pos, It first, It last, std::false_type) {
-    typedef typename std::iterator_traits<It>::iterator_category categ;
+    using categ = typename std::iterator_traits<It>::iterator_category;
+    using it_ref = typename std::iterator_traits<It>::reference;
     if (std::is_same<categ, std::input_iterator_tag>::value) {
       auto offset = pos - begin();
       while (first != last) {
@@ -929,13 +961,20 @@ class small_vector : public detail::small_vector_base<
       return begin() + offset;
     }
 
-    auto distance = std::distance(first, last);
-    auto offset = pos - begin();
+    auto const distance = std::distance(first, last);
+    auto const offset = pos - begin();
+    assert(distance >= 0);
+    assert(offset >= 0);
     makeSize(size() + distance);
-    detail::moveObjectsRight(
-        data() + offset, data() + size(), data() + size() + distance);
+    detail::moveObjectsRightAndCreate(
+        data() + offset,
+        data() + size(),
+        data() + size() + distance,
+        [&](size_t i) -> it_ref {
+          assert(i < size_t(distance));
+          return *(first + i);
+        });
     this->setSize(size() + distance);
-    std::copy_n(first, distance, begin() + offset);
     return begin() + offset;
   }
 

folly/test/small_vector_test.cpp

@@ -329,7 +329,7 @@ TEST(small_vector, leak_test) {
   }
 }
 
-TEST(small_vector, Insert) {
+TEST(small_vector, InsertTrivial) {
   folly::small_vector<int> someVec(3, 3);
   someVec.insert(someVec.begin(), 12, 12);
   EXPECT_EQ(someVec.size(), 15);
@@ -341,15 +341,38 @@ TEST(small_vector, Insert) {
     }
   }
 
+  // Make sure we insert a larger range so we can test placement new
+  // and move inserts
   auto oldSize = someVec.size();
-  someVec.insert(someVec.begin() + 1, 12, 12);
-  EXPECT_EQ(someVec.size(), oldSize + 12);
+  someVec.insert(someVec.begin() + 1, 30, 30);
+  EXPECT_EQ(someVec.size(), oldSize + 30);
+  EXPECT_EQ(someVec[0], 12);
+  EXPECT_EQ(someVec[1], 30);
+  EXPECT_EQ(someVec[31], 12);
+}
 
+TEST(small_vector, InsertNontrivial) {
   folly::small_vector<std::string> v1(6, "asd"), v2(7, "wat");
   v1.insert(v1.begin() + 1, v2.begin(), v2.end());
   EXPECT_TRUE(v1.size() == 6 + 7);
   EXPECT_EQ(v1.front(), "asd");
   EXPECT_EQ(v1[1], "wat");
+
+  // Insert without default constructor
+  class TestClass {
+   public:
+    // explicit TestClass() = default;
+    explicit TestClass(std::string s) : s(s) {}
+    std::string s;
+  };
+  folly::small_vector<TestClass> v3(5, TestClass("asd"));
+  folly::small_vector<TestClass> v4(10, TestClass("wat"));
+  v3.insert(v3.begin() + 1, v4.begin(), v4.end());
+  EXPECT_TRUE(v3.size() == 5 + 10);
+  EXPECT_EQ(v3[0].s, "asd");
+  EXPECT_EQ(v3[1].s, "wat");
+  EXPECT_EQ(v3[10].s, "wat");
+  EXPECT_EQ(v3[11].s, "asd");
 }
 
 TEST(small_vector, Swap) {
@@ -784,6 +807,23 @@ TEST(small_vector, SelfInsert) {
     EXPECT_EQ(vec[i - 1], "abc");
     EXPECT_EQ(vec[i], "abc");
   }
+
+  // range insert
+  for (int i = 2; i < 33; ++i) {
+    folly::small_vector<std::string> vec;
+    // reserve 2 * i space so we don't grow and invalidate references.
+    vec.reserve(2 * i);
+    for (int j = 0; j < i; ++j) {
+      vec.push_back("abc");
+    }
+    EXPECT_EQ(vec.size(), i);
+    vec.insert(vec.end() - 1, vec.begin(), vec.end() - 1);
+    EXPECT_EQ(vec.size(), 2 * i - 1);
+
+    for (auto const& val : vec) {
+      EXPECT_EQ(val, "abc");
+    }
+  }
 }
 
 struct CheckedInt {