new small_vector tests that fail on trunk and uncover a bug in emplace_back on refere...

Summary: emplace_back() on a small_vector applied on data inside the vector doesn't work properly.
In standard vectors, this usage is required to work properly, but I'm not sure whether it should in small_vector. Consider fixing / adding a lint rule.

Reviewed By: @yfeldblum

Differential Revision: D2122931

diff --git a/folly/small_vector.h b/folly/small_vector.h
index a4af2fc4ba8c76c7ab625b8d4fd001414b0ed3f6..318fcbfdf7ece46dc425c9ef5abbe484a1d87493 100644 (file)
--- a/folly/small_vector.h
+++ b/folly/small_vector.h
@@ -658,6 +658,17 @@ public:
     emplaceBack(std::forward<Args>(args)...);
   }
 
+  void emplace_back(const value_type& t) {
+    push_back(t);
+  }
+  void emplace_back(value_type& t) {
+    push_back(t);
+  }
+
+  void emplace_back(value_type&& t) {
+    push_back(std::move(t));
+  }
+
   void push_back(value_type&& t) {
     if (capacity() == size()) {
       makeSize(std::max(size_type(2), 3 * size() / 2), &t, size());
@@ -668,9 +679,17 @@ public:
   }
 
   void push_back(value_type const& t) {
-    // Make a copy and forward to the rvalue value_type&& overload
-    // above.
-    push_back(value_type(t));
+    // TODO: we'd like to make use of makeSize (it can be optimized better,
+    // because it manipulates the internals)
+    // unfortunately the current implementation only supports moving from
+    // a supplied rvalue, and doing an extra move just to reuse it is a perf
+    // net loss
+    if (size() == capacity()) {// && isInside(&t)) {
+      value_type tmp(t);
+      emplaceBack(std::move(tmp));
+    } else {
+      emplaceBack(t);
+    }
   }
 
   void pop_back() {
@@ -809,13 +828,6 @@ private:
     this->setSize(size() + 1);
   }
 
-  /*
-   * Special case of emplaceBack for rvalue
-   */
-  void emplaceBack(value_type&& t) {
-    push_back(std::move(t));
-  }
-
   static iterator unconst(const_iterator it) {
     return const_cast<iterator>(it);
   }

diff --git a/folly/test/small_vector_test.cpp b/folly/test/small_vector_test.cpp
index 860fddc6b49ef1f777198d4c7bdc3611396fde44..a5365393fb2801df482d11e328088948fc545bba 100644 (file)
--- a/folly/test/small_vector_test.cpp
+++ b/folly/test/small_vector_test.cpp
@@ -733,3 +733,72 @@ TEST(small_vector, SelfInsert) {
     EXPECT_EQ(vec[i], "abc");
   }
 }
+
+struct CheckedInt {
+  static const int DEFAULT_VALUE = (int)0xdeadbeef;
+  CheckedInt(): value(DEFAULT_VALUE) {}
+  explicit CheckedInt(int value): value(value) {}
+  CheckedInt(const CheckedInt& rhs): value(rhs.value) {}
+  CheckedInt(CheckedInt&& rhs) noexcept: value(rhs.value) {
+    rhs.value = DEFAULT_VALUE;
+  }
+  CheckedInt& operator= (const CheckedInt& rhs) {
+    value = rhs.value;
+    return *this;
+  }
+  CheckedInt& operator= (CheckedInt&& rhs) noexcept {
+    value = rhs.value;
+    rhs.value = DEFAULT_VALUE;
+    return *this;
+  }
+  ~CheckedInt() {}
+  int value;
+};
+
+TEST(small_vector, LVEmplaceInsideVector) {
+  folly::small_vector<CheckedInt> v;
+  v.push_back(CheckedInt(1));
+  for (int i = 1; i < 20; ++i) {
+    v.emplace_back(v[0]);
+    ASSERT_EQ(1, v.back().value);
+  }
+}
+
+TEST(small_vector, CLVEmplaceInsideVector) {
+  folly::small_vector<CheckedInt> v;
+  const folly::small_vector<CheckedInt>& cv = v;
+  v.push_back(CheckedInt(1));
+  for (int i = 1; i < 20; ++i) {
+    v.emplace_back(cv[0]);
+    ASSERT_EQ(1, v.back().value);
+  }
+}
+
+TEST(small_vector, RVEmplaceInsideVector) {
+  folly::small_vector<CheckedInt> v;
+  v.push_back(CheckedInt(0));
+  for (int i = 1; i < 20; ++i) {
+    v[0] = CheckedInt(1);
+    v.emplace_back(std::move(v[0]));
+    ASSERT_EQ(1, v.back().value);
+  }
+}
+
+TEST(small_vector, LVPushValueInsideVector) {
+  folly::small_vector<CheckedInt> v;
+  v.push_back(CheckedInt(1));
+  for (int i = 1; i < 20; ++i) {
+    v.push_back(v[0]);
+    ASSERT_EQ(1, v.back().value);
+  }
+}
+
+TEST(small_vector, RVPushValueInsideVector) {
+  folly::small_vector<CheckedInt> v;
+  v.push_back(CheckedInt(0));
+  for (int i = 1; i < 20; ++i) {
+    v[0] = CheckedInt(1);
+    v.push_back(v[0]);
+    ASSERT_EQ(1, v.back().value);
+  }
+}