From 97b18021f675333d2fde67fb13508c90a4654837 Mon Sep 17 00:00:00 2001
From: Andrei-Florin BENCSIK <andrei.bencsik@gmail.com>
Date: Sun, 22 Sep 2019 10:01:30 +0300
Subject: [PATCH] Remove unused function and add more UTs

When browsing around the strutil files I found a function
that was never referenced inside the code base
"void StripString(string* s, const char* remove,
-                                 char replacewith);"

The name was kind of misleading as well and it seems like
it's a carbon copy of
"void ReplaceCharacters(string* s, const char* remove,
                                       char replacewith);"
(even the parameter names are the same, the code is the same..)

Is it intentional? Maybe for compatibility reasons? If so,
let's make it deprecated and use the ReplaceCharacters method inside
or the other way around.

Also, noticed there were no tests for "StripString" or "Replace".
Added some for both and planning on maybe making it more C++ish (?)
in another commit.
---
 src/google/protobuf/stubs/strutil.cc          | 15 -------
 src/google/protobuf/stubs/strutil.h           |  2 -
 src/google/protobuf/stubs/strutil_unittest.cc | 44 +++++++++++++++++++
 3 files changed, 44 insertions(+), 17 deletions(-)

diff --git a/src/google/protobuf/stubs/strutil.cc b/src/google/protobuf/stubs/strutil.cc
index 5486887295..06250b1dd3 100644
--- a/src/google/protobuf/stubs/strutil.cc
+++ b/src/google/protobuf/stubs/strutil.cc
@@ -80,21 +80,6 @@ inline bool isprint(char c) {
   return c >= 0x20 && c <= 0x7E;
 }
 
-// ----------------------------------------------------------------------
-// StripString
-//    Replaces any occurrence of the character 'remove' (or the characters
-//    in 'remove') with the character 'replacewith'.
-// ----------------------------------------------------------------------
-void StripString(string* s, const char* remove, char replacewith) {
-  const char * str_start = s->c_str();
-  const char * str = str_start;
-  for (str = strpbrk(str, remove);
-       str != nullptr;
-       str = strpbrk(str + 1, remove)) {
-    (*s)[str - str_start] = replacewith;
-  }
-}
-
 // ----------------------------------------------------------------------
 // ReplaceCharacters
 //    Replaces any occurrence of the character 'remove' (or the characters
diff --git a/src/google/protobuf/stubs/strutil.h b/src/google/protobuf/stubs/strutil.h
index 790500d26e..e28d22ec8b 100644
--- a/src/google/protobuf/stubs/strutil.h
+++ b/src/google/protobuf/stubs/strutil.h
@@ -159,8 +159,6 @@ inline string StripSuffixString(const string& str, const string& suffix) {
 // ----------------------------------------------------------------------
 PROTOBUF_EXPORT void ReplaceCharacters(string* s, const char* remove,
                                        char replacewith);
-PROTOBUF_EXPORT void StripString(string* s, const char* remove,
-                                 char replacewith);
 
 PROTOBUF_EXPORT void StripWhitespace(string* s);
 
diff --git a/src/google/protobuf/stubs/strutil_unittest.cc b/src/google/protobuf/stubs/strutil_unittest.cc
index f830e37704..07d16efacd 100644
--- a/src/google/protobuf/stubs/strutil_unittest.cc
+++ b/src/google/protobuf/stubs/strutil_unittest.cc
@@ -836,6 +836,50 @@ TEST(StrCat, Ints) {
   EXPECT_EQ(answer, "130");
 }
 
+class ReplaceChars : public ::testing::TestWithParam<std::tuple<string, string, const char*, char>> {
+};
+
+TEST_P(ReplaceChars, ReplacesAllOccurencesOfAnyCharInReplaceWithAReplaceChar) {
+    string expected = std::get<0>(GetParam());
+    string string_to_replace_in = std::get<1>(GetParam());
+    const char* what_to_replace = std::get<2>(GetParam());
+    char replacement = std::get<3>(GetParam());
+    ReplaceCharacters(&string_to_replace_in, what_to_replace, replacement);
+    ASSERT_EQ(expected, string_to_replace_in);
+}
+
+INSTANTIATE_TEST_CASE_P(Replace,
+    ReplaceChars,
+    ::testing::Values(
+        std::make_tuple("", "", "", '_'), // empty string should remain empty
+        std::make_tuple(" ", " ", "", '_'), // no replacement string
+        std::make_tuple(" ", " ", "_-abcedf", '*'), // replacement character not in string
+        std::make_tuple("replace", "Replace", "R", 'r'), // replace one character
+        std::make_tuple("not_spaces__", "not\nspaces\t ", " \t\r\n", '_'), // replace some special characters
+        std::make_tuple("c++", "cxx", "x", '+'), // same character multiple times
+        std::make_tuple("qvvvvvng v T", "queueing a T", "aeiou", 'v'))); // replace all voewls
+
+class StripWs: public ::testing::TestWithParam<std::tuple<string, string>> {
+};
+
+TEST_P(StripWs, AlwaysStripsLeadingAndTrailingWhitespace) {
+    string expected = std::get<0>(GetParam());
+    string string_to_strip = std::get<1>(GetParam());
+    StripWhitespace(&string_to_strip);
+    ASSERT_EQ(expected, string_to_strip);
+}
+
+INSTANTIATE_TEST_CASE_P(Strip,
+    StripWs,
+    ::testing::Values(
+        std::make_tuple("", ""), // empty string should remain empty
+        std::make_tuple("", " "), // only ws should become empty
+        std::make_tuple("no whitespace", " no whitespace"), // leading ws removed
+        std::make_tuple("no whitespace", "no whitespace "), // trailing ws removed
+        std::make_tuple("no whitespace", " no whitespace "), // same nb. of leading and trailing
+        std::make_tuple("no whitespace", "  no whitespace "), // different nb. of leading/trailing
+        std::make_tuple("no whitespace", " no whitespace  "))); // more trailing than leading
+
 }  // anonymous namespace
 }  // namespace protobuf
 }  // namespace google