[string_util] fix bug in ReplaceSubstringsAfterOffset()

base/strings/string_util_unittest.cc

Index: base/strings/string_util_unittest.cc
diff --git a/base/strings/string_util_unittest.cc b/base/strings/string_util_unittest.cc
index 6ac307ec2b4f1113d53af03ef4a655f2b1f32657..c9fefbb967630c91796622a11214d85d305275d4 100644
--- a/base/strings/string_util_unittest.cc
+++ b/base/strings/string_util_unittest.cc
@@ -591,18 +591,27 @@ TEST(StringUtilTest, ReplaceSubstringsAfterOffset) {
     const char* replace_with;
     const char* expected;
   } cases[] = {
-    {"aaa", 0, "a", "b", "bbb"},
-    {"abb", 0, "ab", "a", "ab"},
-    {"Removing some substrings inging", 0, "ing", "", "Remov some substrs "},
-    {"Not found", 0, "x", "0", "Not found"},
-    {"Not found again", 5, "x", "0", "Not found again"},
-    {" Making it much longer ", 0, " ", "Four score and seven years ago",
-     "Four score and seven years agoMakingFour score and seven years agoit"
-     "Four score and seven years agomuchFour score and seven years agolonger"
-     "Four score and seven years ago"},
-    {"Invalid offset", 9999, "t", "foobar", "Invalid offset"},
-    {"Replace me only me once", 9, "me ", "", "Replace me only once"},
-    {"abababab", 2, "ab", "c", "abccc"},
+      {"aaa", 0, "a", "b", "bbb"},
+      {"aaa", 0, "aa", "b", "ba"},
+      {"aaa", 0, "aa", "bbb", "bbba"},
+      {"aaaaa", 0, "aa", "b", "bba"},
+      {"ababaaababa", 0, "aba", "", "baaba"},
+      {"ababaaababa", 0, "aba", "_", "_baa_ba"},
+      {"ababaaababa", 0, "aba", "__", "__baa__ba"},
+      {"ababaaababa", 0, "aba", "___", "___baa___ba"},
+      {"ababaaababa", 0, "aba", "____", "____baa____ba"},
+      {"ababaaababa", 0, "aba", "_____", "_____baa_____ba"},
+      {"abb", 0, "ab", "a", "ab"},
+      {"Removing some substrings inging", 0, "ing", "", "Remov some substrs "},
+      {"Not found", 0, "x", "0", "Not found"},
+      {"Not found again", 5, "x", "0", "Not found again"},
+      {" Making it much longer ", 0, " ", "Four score and seven years ago",
+       "Four score and seven years agoMakingFour score and seven years agoit"
+       "Four score and seven years agomuchFour score and seven years agolonger"
+       "Four score and seven years ago"},
+      {"Invalid offset", 9999, "t", "foobar", "Invalid offset"},
+      {"Replace me only me once", 9, "me ", "", "Replace me only once"},
+      {"abababab", 2, "ab", "c", "abccc"},
   };
 
   for (size_t i = 0; i < arraysize(cases); i++) {
@@ -612,6 +621,26 @@ TEST(StringUtilTest, ReplaceSubstringsAfterOffset) {
                                  ASCIIToUTF16(cases[i].replace_with));
     EXPECT_EQ(ASCIIToUTF16(cases[i].expected), str);
   }
+
+  for (size_t i = 0; i < arraysize(cases); i++) {

[Peter Kasting, 2017/07/27 05:25:14]

Nit: Use range-based for (2 places)

[ncarter (slow), 2017/07/28 02:34:25]

Done.

+    // Insufficient capacity case; will realloc the string.
+    std::string str = cases[i].str;
+    str.shrink_to_fit();

[Peter Kasting, 2017/07/27 05:25:14]

Note that shrink_to_fit() is a non-binding request.  It may not actually reduce
the capacity.  The comments should probably note that.  If it's critical to the
test, I don't know how to force this, since basic_string is intentionally pretty
unwilling to let people force specific capacities.

[ncarter (slow), 2017/07/28 02:34:25]

Done.

+    ReplaceSubstringsAfterOffset(&str, cases[i].start_offset,
+                                 cases[i].find_this, cases[i].replace_with);
+    EXPECT_EQ(cases[i].expected, str);
+  }
+
+  for (size_t i = 0; i < arraysize(cases); i++) {
+    // Ample capacity case; will try to in place

[Peter Kasting, 2017/07/27 05:25:14]

Nit: Will try to ____ in place?

[ncarter (slow), 2017/07/28 02:34:25]

Done.

+    std::string str = cases[i].str;
+    str.reserve(strlen(cases[i].expected) * 2);
+    const void* original_buffer = str.data();

[Peter Kasting, 2017/07/27 05:25:14]

Nit: Why not char*?

That said, I'm uncomfortable unittesting this.  It's not part of Replace...'s
contract that it doesn't realloc.  I think the test should verify behavioral
correctness per the contract and not try to verify that we're not doing a
realloc.

[ncarter (slow), 2017/07/28 02:34:25]

Removed.

+    ReplaceSubstringsAfterOffset(&str, cases[i].start_offset,
+                                 cases[i].find_this, cases[i].replace_with);
+    EXPECT_EQ(cases[i].expected, str);
+    EXPECT_EQ(original_buffer, str.data());
+  }
 }
 
 TEST(StringUtilTest, ReplaceFirstSubstringAfterOffset) {