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

diff --git a/folly/FBString.h b/folly/FBString.h
index 06b8b86e2236f5e7a3eef2d0f13d00fedcfcd7a8..d26b961d2a2bc325334885a7044ecb4fb28b98a7 100644 (file)
--- a/folly/FBString.h
+++ b/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;

diff --git a/folly/test/FBStringTest.cpp b/folly/test/FBStringTest.cpp
index 15783ea1b6d60dd6d9b8af1dc074a566718dcf66..94bbac992d91665e151accc88c52e9f70560d285 100644 (file)
--- a/folly/test/FBStringTest.cpp
+++ b/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) {