[string_util] fix bug in ReplaceSubstringsAfterOffset()

base/strings/string_util_unittest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/strings/string_util.h"
 
 #include <math.h>
 #include <stdarg.h>
 #include <stddef.h>
 #include <stdint.h>
@@ skipping 573 matching lines @@
   }
 }
 TEST(StringUtilTest, ReplaceSubstringsAfterOffset) {
   static const struct {
     const char* str;
     string16::size_type start_offset;
     const char* find_this;
     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++) {
     string16 str = ASCIIToUTF16(cases[i].str);
     ReplaceSubstringsAfterOffset(&str, cases[i].start_offset,
                                  ASCIIToUTF16(cases[i].find_this),
                                  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) {
   static const struct {
     const char* str;
     string16::size_type start_offset;
     const char* find_this;
     const char* replace_with;
     const char* expected;
   } cases[] = {
@@ skipping 606 matching lines @@
   const std::string live = kLive;
   std::string dead = live;
   strncpy(WriteInto(&dead, 5), kDead, 4);
   EXPECT_EQ(kDead, dead);
   EXPECT_EQ(4u, dead.size());
   EXPECT_EQ(kLive, live);
   EXPECT_EQ(4u, live.size());
 }
 
 }  // namespace base