Avoid string copying in json.cc.

The following two methods:

  bool Json::HasStringValueForKey(const std::string& key) const;
  std::string Json::GetStringValueForKey(const std::string& key) const;

have been replaced with a single method:

  bool Json::GetStringValueForKey(const std::string& key,
                                  std::string* value) const;

to avoid copying possibly large strings.

BUG=8
R=roubert@google.com

Review URL: https://codereview.appspot.com/112940044

5 files changed, 70 insertions, 89 deletions

diff --git a/cpp/src/preload_supplier.cc b/cpp/src/preload_supplier.cc
index 31325bd..c613d3c 100644
--- a/cpp/src/preload_supplier.cc
+++ b/cpp/src/preload_supplier.cc
@@ -105,6 +105,7 @@ class Helper {
     (void)status;  // Prevent unused variable if assert() is optimized away.
 
     Json json;
+    std::string id;
     std::vector<const Rule*> sub_rules;
 
     if (!success) {
@@ -124,11 +125,10 @@ class Helper {
       }
       const Json& value = json.GetDictionaryValueForKey(*it);
 
-      if (!value.HasStringValueForKey("id")) {
+      if (!value.GetStringValueForKey("id", &id)) {
         success = false;
         goto callback;
       }
-      const std::string& id = value.GetStringValueForKey("id");
       assert(*it == id);  // Sanity check.
 
       size_t depth = std::count(id.begin(), id.end(), '/') - 1;
diff --git a/cpp/src/rule.cc b/cpp/src/rule.cc
index b4d03ca..4d1cff5 100644
--- a/cpp/src/rule.cc
+++ b/cpp/src/rule.cc
@@ -38,20 +38,6 @@ namespace {
 
 typedef std::map<std::string, int> NameMessageIdMap;
 
-const char kAdminAreaNameTypeKey[] = "state_name_type";
-const char kFormatKey[] = "fmt";
-const char kIdKey[] = "id";
-const char kLanguagesKey[] = "languages";
-const char kLatinFormatKey[] = "lfmt";
-const char kLatinNameKey[] = "lname";
-const char kNameKey[] = "name";
-const char kPostalCodeNameTypeKey[] = "zip_name_type";
-const char kRequireKey[] = "require";
-const char kSubKeysKey[] = "sub_keys";
-const char kZipKey[] = "zip";
-const char kPostalCodeExampleKey[] = "zipex";
-const char kPostServiceUrlKey[] = "posturl";
-
 // Used as a separator in a list of items. For example, the list of supported
 // languages can be "de~fr~it".
 const char kSeparator = '~';
@@ -182,36 +168,33 @@ bool Rule::ParseSerializedRule(const std::string& serialized_rule) {
 }
 
 void Rule::ParseJsonRule(const Json& json) {
-  if (json.HasStringValueForKey(kIdKey)) {
-    id_ = json.GetStringValueForKey(kIdKey);
+  std::string value;
+  if (json.GetStringValueForKey("id", &value)) {
+    id_.swap(value);
   }
 
-  if (json.HasStringValueForKey(kFormatKey)) {
-    ParseFormatRule(json.GetStringValueForKey(kFormatKey), &format_);
+  if (json.GetStringValueForKey("fmt", &value)) {
+    ParseFormatRule(value, &format_);
   }
 
-  if (json.HasStringValueForKey(kLatinFormatKey)) {
-    ParseFormatRule(json.GetStringValueForKey(kLatinFormatKey), &latin_format_);
+  if (json.GetStringValueForKey("lfmt", &value)) {
+    ParseFormatRule(value, &latin_format_);
   }
 
-  if (json.HasStringValueForKey(kRequireKey)) {
-    ParseAddressFieldsRequired(
-        json.GetStringValueForKey(kRequireKey), &required_);
+  if (json.GetStringValueForKey("require", &value)) {
+    ParseAddressFieldsRequired(value, &required_);
   }
 
-  if (json.HasStringValueForKey(kSubKeysKey)) {
-    SplitString(
-        json.GetStringValueForKey(kSubKeysKey), kSeparator, &sub_keys_);
+  if (json.GetStringValueForKey("sub_keys", &value)) {
+    SplitString(value, kSeparator, &sub_keys_);
   }
 
-  if (json.HasStringValueForKey(kLanguagesKey)) {
-    SplitString(
-        json.GetStringValueForKey(kLanguagesKey), kSeparator, &languages_);
+  if (json.GetStringValueForKey("languages", &value)) {
+    SplitString(value, kSeparator, &languages_);
   }
 
   sole_postal_code_.clear();
-  if (json.HasStringValueForKey(kZipKey)) {
-    const std::string& zip = json.GetStringValueForKey(kZipKey);
+  if (json.GetStringValueForKey("zip", &value)) {
     // The "zip" field in the JSON data is used in two different ways to
     // validate the postal code. At the country level, the "zip" field indicates
     // a Java compatible regular expression corresponding to all postal codes in
@@ -225,7 +208,7 @@ void Rule::ParseJsonRule(const Json& json) {
     // RE2::FullMatch() to perform matching against the entire string.
     RE2::Options options;
     options.set_never_capture(true);
-    RE2* matcher = new RE2("^(" + zip + ")", options);
+    RE2* matcher = new RE2("^(" + value + ")", options);
     if (matcher->ok()) {
       postal_code_matcher_.reset(new RE2ptr(matcher));
     } else {
@@ -234,37 +217,35 @@ void Rule::ParseJsonRule(const Json& json) {
     }
     // If the "zip" field is not a regular expression, then it is the sole
     // postal code for this rule.
-    if (!ContainsRegExSpecialCharacters(zip)) {
-      sole_postal_code_ = zip;
+    if (!ContainsRegExSpecialCharacters(value)) {
+      sole_postal_code_.swap(value);
     }
   }
 
-  if (json.HasStringValueForKey(kAdminAreaNameTypeKey)) {
+  if (json.GetStringValueForKey("state_name_type", &value)) {
     admin_area_name_message_id_ =
-        GetMessageIdFromName(json.GetStringValueForKey(kAdminAreaNameTypeKey),
-                             GetAdminAreaMessageIds());
+        GetMessageIdFromName(value, GetAdminAreaMessageIds());
   }
 
-  if (json.HasStringValueForKey(kPostalCodeNameTypeKey)) {
+  if (json.GetStringValueForKey("zip_name_type", &value)) {
     postal_code_name_message_id_ =
-        GetMessageIdFromName(json.GetStringValueForKey(kPostalCodeNameTypeKey),
-                             GetPostalCodeMessageIds());
+        GetMessageIdFromName(value, GetPostalCodeMessageIds());
   }
 
-  if (json.HasStringValueForKey(kNameKey)) {
-    name_ = json.GetStringValueForKey(kNameKey);
+  if (json.GetStringValueForKey("name", &value)) {
+    name_.swap(value);
   }
 
-  if (json.HasStringValueForKey(kLatinNameKey)) {
-    latin_name_ = json.GetStringValueForKey(kLatinNameKey);
+  if (json.GetStringValueForKey("lname", &value)) {
+    latin_name_.swap(value);
   }
 
-  if (json.HasStringValueForKey(kPostalCodeExampleKey)) {
-    postal_code_example_ = json.GetStringValueForKey(kPostalCodeExampleKey);
+  if (json.GetStringValueForKey("zipex", &value)) {
+    postal_code_example_.swap(value);
   }
 
-  if (json.HasStringValueForKey(kPostServiceUrlKey)) {
-    post_service_url_ = json.GetStringValueForKey(kPostServiceUrlKey);
+  if (json.GetStringValueForKey("posturl", &value)) {
+    post_service_url_.swap(value);
   }
 }
 
diff --git a/cpp/src/util/json.cc b/cpp/src/util/json.cc
index d819bb2..352c62b 100644
--- a/cpp/src/util/json.cc
+++ b/cpp/src/util/json.cc
@@ -139,23 +139,19 @@ const std::vector<std::string>& Json::GetKeys() const {
   return impl_->GetKeys();
 }
 
-bool Json::HasStringValueForKey(const std::string& key) const {
-  assert(impl_ != NULL);
-
-  // Member is owned by impl_.
-  const Value::Member* member = impl_->FindMember(key);
-  return member != NULL && member->value.IsString();
-}
-
-std::string Json::GetStringValueForKey(const std::string& key) const {
+bool Json::GetStringValueForKey(const std::string& key,
+                                std::string* value) const {
   assert(impl_ != NULL);
+  assert(value != NULL);
 
   // Member is owned by impl_.
   const Value::Member* member = impl_->FindMember(key.c_str());
-  assert(member != NULL);
-  assert(member->value.IsString());
-  return std::string(member->value.GetString(),
-                     member->value.GetStringLength());
+  if (member == NULL || !member->value.IsString()) {
+    return false;
+  }
+
+  value->assign(member->value.GetString(), member->value.GetStringLength());
+  return true;
 }
 
 bool Json::HasDictionaryValueForKey(const std::string& key) const {
diff --git a/cpp/src/util/json.h b/cpp/src/util/json.h
index 1fbf500..90565e6 100644
--- a/cpp/src/util/json.h
+++ b/cpp/src/util/json.h
@@ -43,15 +43,11 @@ class Json {
   // successfully in ParseObject() before invoking this method.
   const std::vector<std::string>& GetKeys() const;
 
-  // Returns true if the parsed JSON contains a string value for |key|. The JSON
-  // object must be parsed successfully in ParseObject() before invoking this
-  // method.
-  bool HasStringValueForKey(const std::string& key) const;
-
-  // Returns the string value for the |key|. The |key| must be present and its
-  // value must be of string type, i.e., HasStringValueForKey(key) must return
-  // true before invoking this method.
-  std::string GetStringValueForKey(const std::string& key) const;
+  // Returns true if the parsed JSON contains a string value for |key|. Sets
+  // |value| to the string value of the |key|. The JSON object must be parsed
+  // successfully in ParseObject() before invoking this method. The |value|
+  // parameter should not be NULL.
+  bool GetStringValueForKey(const std::string& key, std::string* value) const;
 
   // Returns true if the parsed JSON contains a dictionary value for |key|. The
   // JSON object must be parsed successfully in ParseObject() before invoking
diff --git a/cpp/test/util/json_test.cc b/cpp/test/util/json_test.cc
index f6eb389..a3b9f80 100644
--- a/cpp/test/util/json_test.cc
+++ b/cpp/test/util/json_test.cc
@@ -32,8 +32,9 @@ TEST(JsonTest, EmptyStringIsNotValid) {
 TEST(JsonTest, EmptyDictionaryContainsNoKeys) {
   Json json;
   ASSERT_TRUE(json.ParseObject("{}"));
-  EXPECT_FALSE(json.HasStringValueForKey("key"));
-  EXPECT_FALSE(json.HasStringValueForKey(std::string()));
+  std::string not_checked;
+  EXPECT_FALSE(json.GetStringValueForKey("key", &not_checked));
+  EXPECT_FALSE(json.GetStringValueForKey(std::string(), &not_checked));
 }
 
 TEST(JsonTest, InvalidJsonIsNotValid) {
@@ -44,35 +45,40 @@ TEST(JsonTest, InvalidJsonIsNotValid) {
 TEST(JsonTest, OneKeyIsValid) {
   Json json;
   ASSERT_TRUE(json.ParseObject("{\"key\": \"value\"}"));
-  ASSERT_TRUE(json.HasStringValueForKey("key"));
-  EXPECT_EQ("value", json.GetStringValueForKey("key"));
+  std::string value;
+  EXPECT_TRUE(json.GetStringValueForKey("key", &value));
+  EXPECT_EQ("value", value);
 }
 
 TEST(JsonTest, EmptyStringKeyIsNotInObject) {
   Json json;
   ASSERT_TRUE(json.ParseObject("{\"key\": \"value\"}"));
-  EXPECT_FALSE(json.HasStringValueForKey(std::string()));
+  std::string not_checked;
+  EXPECT_FALSE(json.GetStringValueForKey(std::string(), &not_checked));
 }
 
 TEST(JsonTest, EmptyKeyIsValid) {
   Json json;
   ASSERT_TRUE(json.ParseObject("{\"\": \"value\"}"));
-  ASSERT_TRUE(json.HasStringValueForKey(std::string()));
-  EXPECT_EQ("value", json.GetStringValueForKey(std::string()));
+  std::string value;
+  EXPECT_TRUE(json.GetStringValueForKey(std::string(), &value));
+  EXPECT_EQ("value", value);
 }
 
 TEST(JsonTest, EmptyValueIsValid) {
   Json json;
   ASSERT_TRUE(json.ParseObject("{\"key\": \"\"}"));
-  ASSERT_TRUE(json.HasStringValueForKey("key"));
-  EXPECT_TRUE(json.GetStringValueForKey("key").empty());
+  std::string value;
+  EXPECT_TRUE(json.GetStringValueForKey("key", &value));
+  EXPECT_TRUE(value.empty());
 }
 
 TEST(JsonTest, Utf8EncodingIsValid) {
   Json json;
   ASSERT_TRUE(json.ParseObject("{\"key\": \"Ü\"}"));
-  ASSERT_TRUE(json.HasStringValueForKey("key"));
-  EXPECT_EQ("Ü", json.GetStringValueForKey("key"));
+  std::string value;
+  EXPECT_TRUE(json.GetStringValueForKey("key", &value));
+  EXPECT_EQ("Ü", value);
 }
 
 TEST(JsonTest, InvalidUtf8IsNotValid) {
@@ -89,11 +95,12 @@ TEST(JsonTest, NullInMiddleIsNotValid) {
 TEST(JsonTest, TwoKeysAreValid) {
   Json json;
   ASSERT_TRUE(json.ParseObject("{\"key1\": \"value1\", \"key2\": \"value2\"}"));
-  ASSERT_TRUE(json.HasStringValueForKey("key1"));
-  EXPECT_EQ("value1", json.GetStringValueForKey("key1"));
+  std::string value;
+  EXPECT_TRUE(json.GetStringValueForKey("key1", &value));
+  EXPECT_EQ("value1", value);
 
-  ASSERT_TRUE(json.HasStringValueForKey("key2"));
-  EXPECT_EQ("value2", json.GetStringValueForKey("key2"));
+  EXPECT_TRUE(json.GetStringValueForKey("key2", &value));
+  EXPECT_EQ("value2", value);
 }
 
 TEST(JsonTest, ListIsNotValid) {
@@ -122,8 +129,9 @@ TEST(JsonTest, DictionaryFound) {
   ASSERT_TRUE(json.ParseObject("{\"key\":{\"inner_key\":\"value\"}}"));
   ASSERT_TRUE(json.HasDictionaryValueForKey("key"));
   const Json& sub_json = json.GetDictionaryValueForKey("key");
-  ASSERT_TRUE(sub_json.HasStringValueForKey("inner_key"));
-  EXPECT_EQ("value", sub_json.GetStringValueForKey("inner_key"));
+  std::string value;
+  EXPECT_TRUE(sub_json.GetStringValueForKey("inner_key", &value));
+  EXPECT_EQ("value", value);
 }
 
 TEST(JsonTest, DictionariesHaveKeys) {