Cleanup SplitStringIntoKeyaluePairs implementation.

base/strings/string_split.cc

 // Copyright (c) 2012 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_split.h"
 
 #include "base/logging.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/third_party/icu/icu_utf.h"
 
 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();
   for (size_t i = 0; i <= c; ++i) {
     if (i == c || str[i] == s) {
       STR tmp(str, last, i - last);
       if (trim_whitespace)
         TrimWhitespace(tmp, TRIM_ALL, &tmp);
       // Avoid converting an empty or all-whitespace source string into a vector
       // of one empty string.
       if (i != c || !r->empty() || !tmp.empty())
         r->push_back(tmp);
       last = i + 1;
     }
   }
 }
 
+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) {
   DCHECK(CBU16_IS_SINGLE(c));
   SplitStringT(str, c, true, r);
 }
 
 void SplitString(const std::string& str,
                  char c,
                  std::vector<std::string>* r) {
 #if CHAR_MIN < 0
   DCHECK(c >= 0);
 #endif
   DCHECK(c < 0x7F);
   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,
                                   StringPairs* key_value_pairs) {
   key_value_pairs->clear();
 
   std::vector<std::string> pairs;
   SplitString(line, key_value_pair_delimiter, &pairs);
 
   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;
 }
 
 template <typename STR>
 static void SplitStringUsingSubstrT(const STR& str,
                                     const STR& s,
                                     std::vector<STR>* r) {
   r->clear();
   typename STR::size_type begin_index = 0;
@@ skipping 89 matching lines @@
                                 std::vector<string16>* result) {
   SplitStringAlongWhitespaceT(str, result);
 }
 
 void SplitStringAlongWhitespace(const std::string& str,
                                 std::vector<std::string>* result) {
   SplitStringAlongWhitespaceT(str, result);
 }
 
 }  // namespace base