libaddressinput - reduce number of copies in storage class by 2

base/values.h

Index: base/values.h
diff --git a/base/values.h b/base/values.h
index 8caf8350a8bfc76b00b96044213b55912466031f..cf00f869435462abe17fb6a5f703c7dd22bdd34a 100644
--- a/base/values.h
+++ b/base/values.h
@@ -91,6 +91,7 @@ class BASE_EXPORT Value {
   virtual bool GetAsDouble(double* out_value) const;
   virtual bool GetAsString(std::string* out_value) const;
   virtual bool GetAsString(string16* out_value) const;
+  virtual bool GetAsStringValue(const StringValue** out_value) const;

[brettw, 2014/01/28 03:00:24]

This new name seems good but it doesn't follow the pattern of the rest of the
functions. I think for consistency this should be just "GetAsString" (or feel
free to change the other ones!).

   virtual bool GetAsList(ListValue** out_value);
   virtual bool GetAsList(const ListValue** out_value) const;
   virtual bool GetAsDictionary(DictionaryValue** out_value);
@@ -152,16 +153,22 @@ class BASE_EXPORT StringValue : public Value {
   // Initializes a StringValue with a string16.
   explicit StringValue(const string16& in_value);
 
+  // Creates a string value that takes ownership of a string.
+  explicit StringValue(scoped_ptr<std::string> in_value);
+
   virtual ~StringValue();
 
   // Overridden from Value:
   virtual bool GetAsString(std::string* out_value) const OVERRIDE;
   virtual bool GetAsString(string16* out_value) const OVERRIDE;
+  virtual bool GetAsStringValue(const StringValue** out_value) const OVERRIDE;
   virtual StringValue* DeepCopy() const OVERRIDE;
   virtual bool Equals(const Value* other) const OVERRIDE;
 
+  const std::string& value() const { return *value_; }
+
  private:
-  std::string value_;
+  scoped_ptr<std::string> value_;

[brettw, 2014/01/28 03:00:24]

I assume you did this to avoid the extra copy when constructing from a passed-in
string for your use-case. But this adds an extra malloc for every existing user
of this class, and generally malloc will be much more expensive than copying a
random string. Are we sure that this is the correct tradeoff?

[Evan Stade, 2014/01/28 18:37:36]

yes
hmm. Do you think I should create a second StringValue class instead?
HeapStringValue or some such?

I've also noticed there's a JSONStringValue class and I'd like to be able to
access its payload without copying, just as I've done here. But GetAsStringValue
won't work for that, so I guess I need GetAsStringRef(std::string** out_value)
instead of GetAsStringValue/value().

[Evan Stade, 2014/01/28 18:43:35]

Actually, it looks like StringPiece can't return a reference to itself as a
string. Let me rethink this.

[Evan Stade, 2014/01/28 18:48:45]

Well, even in lieu of that other optimization, I still think the one  in this
patch is helfpul. So the question, again, is do you want me to make a
HeapStringValue instead?

 };
 
 class BASE_EXPORT BinaryValue: public Value {