Cleanup SplitStringIntoKeyaluePairs implementation.

base/strings/string_split.cc

Index: base/strings/string_split.cc
diff --git a/base/strings/string_split.cc b/base/strings/string_split.cc
index 210789cd22d0df9b524d40aaeca002ad6e6fb705..daabf840c9d76a6558ed9389aa591b45716e8671 100644
--- a/base/strings/string_split.cc
+++ b/base/strings/string_split.cc
@@ -11,11 +11,13 @@
 
 namespace base {
 
-template<typename STR>
-static void SplitStringT(const STR& str,
-                         const typename STR::value_type s,
-                         bool trim_whitespace,
-                         std::vector<STR>* r) {
+namespace {
+
+template <typename STR>
+void SplitStringT(const STR& str,
+                  const typename STR::value_type s,
+                  bool trim_whitespace,
+                  std::vector<STR>* r) {
   r->clear();
   size_t last = 0;
   size_t c = str.size();
@@ -33,6 +35,34 @@ static void SplitStringT(const STR& str,
   }
 }
 
+bool SplitStringIntoKeyValue(const std::string& line,
+                             char key_value_delimiter,
+                             std::string* key,
+                             std::string* value) {
+  key->clear();
+  value->clear();
+
+  // Find the delimiter.
+  size_t end_key_pos = line.find_first_of(key_value_delimiter);
+  if (end_key_pos == std::string::npos) {
+    DVLOG(1) << "cannot find delimiter in: " << line;
+    return false;    // no delimiter
+  }
+  key->assign(line, 0, end_key_pos);
+
+  // Find the value string.
+  std::string remains(line, end_key_pos, line.size() - end_key_pos);
+  size_t begin_value_pos = remains.find_first_not_of(key_value_delimiter);
+  if (begin_value_pos == std::string::npos) {
+    DVLOG(1) << "cannot parse value from line: " << line;
+    return false;   // no value
+  }
+  value->assign(remains, begin_value_pos, remains.size() - begin_value_pos);
+  return true;
+}
+
+}  // namespace
+
 void SplitString(const string16& str,
                  char16 c,
                  std::vector<string16>* r) {
@@ -50,36 +80,6 @@ void SplitString(const std::string& str,
   SplitStringT(str, c, true, r);
 }
 
-bool SplitStringIntoKeyValues(
-    const std::string& line,
-    char key_value_delimiter,
-    std::string* key, std::vector<std::string>* values) {
-  key->clear();
-  values->clear();
-
-  // Find the key string.
-  size_t end_key_pos = line.find_first_of(key_value_delimiter);
-  if (end_key_pos == std::string::npos) {
-    DVLOG(1) << "cannot parse key from line: " << line;
-    return false;    // no key
-  }
-  key->assign(line, 0, end_key_pos);
-
-  // Find the values string.
-  std::string remains(line, end_key_pos, line.size() - end_key_pos);
-  size_t begin_values_pos = remains.find_first_not_of(key_value_delimiter);
-  if (begin_values_pos == std::string::npos) {
-    DVLOG(1) << "cannot parse value from line: " << line;
-    return false;   // no value
-  }
-  std::string values_string(remains, begin_values_pos,
-                            remains.size() - begin_values_pos);
-
-  // Construct the values vector.
-  values->push_back(values_string);
-  return true;
-}
-
 bool SplitStringIntoKeyValuePairs(const std::string& line,
                                   char key_value_delimiter,
                                   char key_value_pair_delimiter,
@@ -91,23 +91,19 @@ bool SplitStringIntoKeyValuePairs(const std::string& line,
 
   bool success = true;
   for (size_t i = 0; i < pairs.size(); ++i) {
-    // Empty pair. SplitStringIntoKeyValues is more strict about an empty pair
-    // line, so continue with the next pair.
+    // Don't add empty pairs into the result.

[Mark Mentovai, 2014/03/06 17:55:51]

This is empty pairs after whitespace trimming. Desirable?

     if (pairs[i].empty())
       continue;
 
     std::string key;
-    std::vector<std::string> value;
-    if (!SplitStringIntoKeyValues(pairs[i],
-                                  key_value_delimiter,
-                                  &key, &value)) {
-      // Don't return here, to allow for keys without associated
-      // values; just record that our split failed.
+    std::string value;
+    if (!SplitStringIntoKeyValue(pairs[i], key_value_delimiter, &key, &value)) {

[Mark Mentovai, 2014/03/06 17:55:51]

This whole thing would be better with StringPieces, but I’m not going to ask you
to rewrite it using StringPiece because it relies on a bunch of other pieces of
this file that don’t use StringPiece either. I’m just pointing it out because it
would be an easyish area for optimization by removing multiple string copies if
this function is found to be a hot spot.

+      // Don't return here, to allow for pairs without associated
+      // value or key; just record that our split failed.

[Mark Mentovai, 2014/03/06 17:55:51]

our split -> the split

See the test CL. I’m not sure that this behavior is useful.

[pneubeck (no reviews), 2014/03/07 17:12:11]

Done.

       success = false;
     }
     DCHECK_LE(value.size(), 1U);

[Mark Mentovai, 2014/03/06 17:55:51]

?

[pneubeck (no reviews), 2014/03/07 17:12:11]

Ouch. yes that has to be removed.
Done.

-    key_value_pairs->push_back(
-        make_pair(key, value.empty() ? std::string() : value[0]));
+    key_value_pairs->push_back(make_pair(key, value));
   }
   return success;
 }