Fix two fbstring operator+ overloads

Summary: `insert()` returns `fbstring` in most cases, but `iterator` (that is, `value_type*`) when the first argument is an iterator. Two overloads of `operator+` used `insert` as if it returned `fbstring`, which by chance works anyway unless the resulting string contains a `'\0'` (plus it does an extra string copy). This diff fixes the bug.

Reviewed By: philippv, luciang, Gownta

Differential Revision: D2813713

fb-gh-sync-id: 015188b72813da2dabe23980f50f00832d62aa14

folly/FBString.h

@@ -2121,7 +2121,8 @@ basic_fbstring<E, T, A, S> operator+(
   const auto len = basic_fbstring<E, T, A, S>::traits_type::length(lhs);
   if (rhs.capacity() >= len + rhs.size()) {
     // Good, at least we don't need to reallocate
-    return std::move(rhs.insert(rhs.begin(), lhs, lhs + len));
+    rhs.insert(rhs.begin(), lhs, lhs + len);
+    return rhs;
   }
   // Meh, no go. Do it by hand since we have len already.
   basic_fbstring<E, T, A, S> result;
@@ -2153,7 +2154,8 @@ basic_fbstring<E, T, A, S> operator+(
   //
   if (rhs.capacity() > rhs.size()) {
     // Good, at least we don't need to reallocate
-    return std::move(rhs.insert(rhs.begin(), lhs));
+    rhs.insert(rhs.begin(), lhs);
+    return rhs;
   }
   // Meh, no go. Forward to operator+(E, const&).
   auto const& rhsC = rhs;

folly/test/FBStringTest.cpp

@@ -1259,6 +1259,17 @@ TEST(FBString, testFixedBugs) {
     copy.push_back('b');
     EXPECT_GE(copy.capacity(), 1);
   }
+  { // D2813713
+    fbstring s1("a");
+    s1.reserve(8); // Trigger the optimized code path.
+    auto test1 = '\0' + std::move(s1);
+    EXPECT_EQ(2, test1.size());
+
+    fbstring s2(1, '\0');
+    s2.reserve(8);
+    auto test2 = "a" + std::move(s2);
+    EXPECT_EQ(2, test2.size());
+  }
 }
 
 TEST(FBString, findWithNpos) {