Inline StringValue into base::Value

base/values.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/values.h"
 
 #include <string.h>
 
 #include <algorithm>
 #include <cmath>
@@ skipping 69 matching lines @@
 Value::Value(int in_int) : type_(Type::INTEGER), int_value_(in_int) {}
 
 Value::Value(double in_double) : type_(Type::DOUBLE), double_value_(in_double) {
   if (!std::isfinite(double_value_)) {
     NOTREACHED() << "Non-finite (i.e. NaN or positive/negative infinity) "
                  << "values cannot be represented in JSON";
     double_value_ = 0.0;
   }
 }
 
+Value::Value(const char* in_string) : type_(Type::STRING) {
+  string_value_.Init(in_string);
+  DCHECK(IsStringUTF8(*string_value_));
+}
+
+Value::Value(StringPiece in_string) : type_(Type::STRING) {
+  string_value_.Init(in_string.as_string());
+  DCHECK(IsStringUTF8(*string_value_));
+}
+
+Value::Value(const string16& in_string) : type_(Type::STRING) {
+  string_value_.Init(UTF16ToUTF8(in_string));
+}
+
 Value::~Value() {
+  InternalCleanup();
 }
 
 void Value::InternalCopyFrom(const Value& that) {
   type_ = that.type_;
   switch (type_) {
     case Type::NONE:
       // Nothing to do
       return;
 
     case Type::BOOLEAN:
       bool_value_ = that.bool_value_;
       return;
     case Type::INTEGER:
       int_value_ = that.int_value_;
       return;
     case Type::DOUBLE:
       double_value_ = that.double_value_;
       return;
+    case Type::STRING:
+      string_value_.Init(*that.string_value_);
+      return;
 
     // TODO(crbug.com/646113): Implement these once the corresponding derived
     // classes are removed.
-    case Type::STRING:
     case Type::BINARY:
     case Type::LIST:
     case Type::DICTIONARY:
+      return;
+  }
+}
+
+void Value::InternalMoveFrom(Value&& that) {
+  type_ = that.type_;
+
+  switch (type_) {
+    case Type::NONE:
+      // Nothing to do
+      return;
+
+    case Type::BOOLEAN:
+      bool_value_ = that.bool_value_;
+      return;
+    case Type::INTEGER:
+      int_value_ = that.int_value_;
+      return;
+    case Type::DOUBLE:
+      double_value_ = that.double_value_;
+      return;
+    case Type::STRING:
+      string_value_.InitFromMove(std::move(that.string_value_));
+      that.type_ = Type::NONE;

[jdoerrie, 2016/12/19 11:54:09]

I decided against setting the type to NONE here, since this will prevent calling
that.string_value_.Destoy() once it gets out of scope. |that.string_value_| will
be in an unspecified state, but this is in accordance with the standard:

17.6.5.15 Moved-from state of library types                                     
   [lib.types.movedfrom]

Objects of types defined in the C++ standard library may be moved from (12.8).
Move operations may be explicitly specified or implicitly generated. Unless
otherwise specified, such moved-from objects shall be placed in a valid but
unspecified state.

[vabr (Chromium), 2016/12/19 13:00:56]

Acknowledged.

+      return;
+
+    // TODO(crbug.com/646113): Implement these once the corresponding derived
+    // classes are removed.
+    case Type::BINARY:
+    case Type::LIST:
+    case Type::DICTIONARY:
+      return;
+  }
+}
+
+void Value::InternalCleanup() {
+  switch (type_) {
+    case Type::NONE:
+    case Type::BOOLEAN:
+    case Type::INTEGER:
+    case Type::DOUBLE:
+      // Nothing to do
+      return;
+
+    case Type::STRING:
+      string_value_.Destroy();
+      return;
+
+    // TODO(crbug.com/646113): Implement these once the corresponding derived
+    // classes are removed.
+    case Type::BINARY:
+    case Type::LIST:
+    case Type::DICTIONARY:
       return;
   }
 }
 
 // static
 std::unique_ptr<Value> Value::CreateNullValue() {
   return WrapUnique(new Value(Type::NONE));
 }
 
 // static
@@ skipping 15 matching lines @@
 
 double Value::GetDouble() const {
   if (is_double())
     return double_value_;
   if (is_int())
     return int_value_;
   CHECK(false);
   return 0.0;
 }
 
+std::string& Value::GetString() {
+  CHECK(is_string());
+  return *string_value_;
+}
+
+const std::string& Value::GetString() const {
+  CHECK(is_string());
+  return *string_value_;
+}
+
 bool Value::GetAsBinary(const BinaryValue** out_value) const {
   return false;
 }
 
 bool Value::GetAsBoolean(bool* out_value) const {
   if (out_value && is_bool()) {
     *out_value = bool_value_;
     return true;
   }
   return is_bool();
@@ skipping 13 matching lines @@
     return true;
   } else if (out_value && is_int()) {
     // Allow promotion from int to double.
     *out_value = int_value_;
     return true;
   }
   return is_double() || is_int();
 }
 
 bool Value::GetAsString(std::string* out_value) const {
-  return false;
+  if (out_value && is_string()) {
+    *out_value = *string_value_;
+    return true;
+  }
+  return is_string();
 }
 
 bool Value::GetAsString(string16* out_value) const {
-  return false;
+  if (out_value && is_string()) {
+    *out_value = UTF8ToUTF16(*string_value_);
+    return true;
+  }
+  return is_string();
 }
 
 bool Value::GetAsString(const StringValue** out_value) const {
-  return false;
+  if (out_value && is_string()) {
+    *out_value = static_cast<const StringValue*>(this);
+    return true;
+  }
+  return is_string();
 }
 
 bool Value::GetAsList(ListValue** out_value) {
   return false;
 }
 
 bool Value::GetAsList(const ListValue** out_value) const {
   return false;
 }
 
@@ skipping 13 matching lines @@
       return CreateNullValue().release();
 
     // For now, make FundamentalValues for backward-compatibility. Convert to
     // Value when that code is deleted.
     case Type::BOOLEAN:
       return new FundamentalValue(bool_value_);
     case Type::INTEGER:
       return new FundamentalValue(int_value_);
     case Type::DOUBLE:
       return new FundamentalValue(double_value_);
+    // For now, make StringValues for backward-compatibility. Convert to
+    // Value when that code is deleted.
+    case Type::STRING:
+      return new StringValue(*string_value_);
 
     default:
       // All other types should be handled by subclasses.
       NOTREACHED();
       return nullptr;
   }
 }
 
 std::unique_ptr<Value> Value::CreateDeepCopy() const {
   return WrapUnique(DeepCopy());
 }
 
 bool Value::Equals(const Value* other) const {
   if (other->type() != type())
     return false;
 
   switch (type()) {
     case Type::NONE:
       return true;
     case Type::BOOLEAN:
       return bool_value_ == other->bool_value_;
     case Type::INTEGER:
       return int_value_ == other->int_value_;
     case Type::DOUBLE:
       return double_value_ == other->double_value_;
+    // TODO(crbug.com/646113): Simplify this once JSONStringValue is removed.
+    case Type::STRING: {
+      std::string lhs, rhs;
+      return GetAsString(&lhs) && other->GetAsString(&rhs) && lhs == rhs;
+    }
     default:
       // This method should only be getting called for the above types -- all
       // subclasses need to provide their own implementation;.
       NOTREACHED();
       return false;
   }
 }
 
 // static
 bool Value::Equals(const Value* a, const Value* b) {
   if ((a == NULL) && (b == NULL)) return true;
   if ((a == NULL) ^  (b == NULL)) return false;
   return a->Equals(b);
 }
 
 Value::Value(Type type) : type_(type) {}
 
 Value::Value(const Value& that) {
   InternalCopyFrom(that);
 }
 
 Value::Value(Value&& that) {
-  // TODO(crbug.com/646113): Implement InternalMoveFrom for types where moving
-  // and copying differ.
-  InternalCopyFrom(that);
+  InternalMoveFrom(std::move(that));
 }
 
 Value& Value::operator=(const Value& that) {
-  if (this != &that)
+  if (this != &that) {
+    InternalCleanup();

[vabr (Chromium), 2016/12/16 17:00:55]

Having the InternalCleanup here, and using Init inside InternalCopyFrom would
enforce heap allocation of strings even if not needed. Consider:

(1) |a| holds the string "12345"
(2) |b| holds the string "123"
(3) a = b

If you do this with raw std::strings instead of StringValues, the operator= will
make a copy of "123", but it will not need to allocate heap space for that;
instead, it will overwrite the buffer for "12345", which is long enough.

But with the current StringValue:
1. InternalCleanup destroys the existing string and its heap space.
2. InternalCopyFrom initialises the (now empty) a.string_value_ with
b.string_value_, resulting in a heap allocation.

Dropping just InternalCleanup does not work, because then you are calling Init
(=constructor) without Destroy(=destructor) on the original string in |a|. To me
this seems like we should not delegate to a common method and just do the
hand-optimised thing here. That means, switch on the type, and for STRING do:

this.string_value_ = that.string_value_;

[jdoerrie, 2016/12/19 11:54:09]

I agree. Most likely we should act similarly with lists and dicts when they get
implemented.

     InternalCopyFrom(that);
+  }
 
   return *this;
 }
 
 Value& Value::operator=(Value&& that) {
   if (this != &that) {
-    // TODO(crbug.com/646113): Implement InternalMoveFrom for types where moving
-    // and copying differ.
-    InternalCopyFrom(that);
+    InternalCleanup();
+    InternalMoveFrom(std::move(that));

[vabr (Chromium), 2016/12/16 17:00:55]

My comment from the const-ref operator= applies here as well.

[jdoerrie, 2016/12/19 11:54:09]

Acknowledged.

   }
 
   return *this;
 }
 
 ///////////////////// FundamentalValue ////////////////////
 
 FundamentalValue::FundamentalValue(bool in_value) : Value(in_value) {}
 
 FundamentalValue::FundamentalValue(int in_value) : Value(in_value) {}
 
 FundamentalValue::FundamentalValue(double in_value) : Value(in_value) {}
 
 FundamentalValue::~FundamentalValue() {}
 
 ///////////////////// StringValue ////////////////////
 
-StringValue::StringValue(StringPiece in_value)
-    : Value(Type::STRING), value_(in_value.as_string()) {
-  DCHECK(IsStringUTF8(in_value));
-}
+StringValue::StringValue(StringPiece in_value) : Value(in_value) {}
 
-StringValue::StringValue(const string16& in_value)
-    : Value(Type::STRING), value_(UTF16ToUTF8(in_value)) {}
-
-StringValue::~StringValue() {
-}
-
-std::string* StringValue::GetString() {
-  return &value_;
-}
-
-const std::string& StringValue::GetString() const {
-  return value_;
-}
-
-bool StringValue::GetAsString(std::string* out_value) const {
-  if (out_value)
-    *out_value = value_;
-  return true;
-}
-
-bool StringValue::GetAsString(string16* out_value) const {
-  if (out_value)
-    *out_value = UTF8ToUTF16(value_);
-  return true;
-}
-
-bool StringValue::GetAsString(const StringValue** out_value) const {
-  if (out_value)
-    *out_value = this;
-  return true;
-}
-
-StringValue* StringValue::DeepCopy() const {
-  return new StringValue(value_);
-}
-
-bool StringValue::Equals(const Value* other) const {
-  if (other->GetType() != GetType())
-    return false;
-  std::string lhs, rhs;
-  return GetAsString(&lhs) && other->GetAsString(&rhs) && lhs == rhs;
-}
+StringValue::StringValue(const string16& in_value) : Value(in_value) {}
 
 ///////////////////// BinaryValue ////////////////////
 
 BinaryValue::BinaryValue() : Value(Type::BINARY), size_(0) {}
 
 BinaryValue::BinaryValue(std::unique_ptr<char[]> buffer, size_t size)
     : Value(Type::BINARY), buffer_(std::move(buffer)), size_(size) {}
 
 BinaryValue::~BinaryValue() {
 }
@@ skipping 855 matching lines @@
 }
 
 std::ostream& operator<<(std::ostream& out, const Value::Type& type) {
   if (static_cast<int>(type) < 0 ||
       static_cast<size_t>(type) >= arraysize(kTypeNames))
     return out << "Invalid Type (index = " << static_cast<int>(type) << ")";
   return out << Value::GetTypeName(type);
 }
 
 }  // namespace base