From 81295897a3f7ae5b15195ede2a531226472a9ecc Mon Sep 17 00:00:00 2001 From: Giuseppe Ottaviano Date: Thu, 7 Jan 2016 17:34:42 -0800 Subject: [PATCH] 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 | 6 ++++-- folly/test/FBStringTest.cpp | 11 +++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/folly/FBString.h b/folly/FBString.h index 06b8b86e..d26b961d 100644 --- a/folly/FBString.h +++ b/folly/FBString.h @@ -2121,7 +2121,8 @@ basic_fbstring operator+( const auto len = basic_fbstring::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 result; @@ -2153,7 +2154,8 @@ basic_fbstring 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 15783ea1..94bbac99 100644 --- 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) { -- 2.34.1