Move Storage for ListValue and DictValue in Union

base/values.h

Index: base/values.h
diff --git a/base/values.h b/base/values.h
index 9b4ebefe3d2a844fd6ec9d0892bdf4bc7e51474b..76399c262bd6741a460f0fe5af4f2b1759b3a76d 100644
--- a/base/values.h
+++ b/base/values.h
@@ -50,6 +50,9 @@ using BinaryValue = Value;
 // See the file-level comment above for more information.
 class BASE_EXPORT Value {
  public:
+  using DictStorage = std::map<std::string, std::unique_ptr<Value>>;
+  using ListStorage = std::vector<std::unique_ptr<Value>>;
+
   enum class Type {
     NONE = 0,
     BOOLEAN,
@@ -99,7 +102,7 @@ class BASE_EXPORT Value {
   Value& operator=(const Value& that);
   Value& operator=(Value&& that);
 
-  virtual ~Value();
+  ~Value();
 
   // Returns the name for a given |type|.
   static const char* GetTypeName(Type type);
@@ -136,45 +139,40 @@ class BASE_EXPORT Value {
   // If the current object can be converted into the given type, the value is
   // returned through the |out_value| parameter and true is returned;
   // otherwise, false is returned and |out_value| is unchanged.
-  virtual bool GetAsBoolean(bool* out_value) const;
-  virtual bool GetAsInteger(int* out_value) const;
-  virtual bool GetAsDouble(double* out_value) const;
-  virtual bool GetAsString(std::string* out_value) const;
-  virtual bool GetAsString(string16* out_value) const;
-  virtual bool GetAsString(const StringValue** out_value) const;
-  virtual bool GetAsString(StringPiece* out_value) const;
-  virtual bool GetAsBinary(const BinaryValue** out_value) const;
+  bool GetAsBoolean(bool* out_value) const;
+  bool GetAsInteger(int* out_value) const;
+  bool GetAsDouble(double* out_value) const;
+  bool GetAsString(std::string* out_value) const;
+  bool GetAsString(string16* out_value) const;
+  bool GetAsString(const StringValue** out_value) const;
+  bool GetAsString(StringPiece* out_value) const;
+  bool GetAsBinary(const BinaryValue** out_value) const;
   // ListValue::From is the equivalent for std::unique_ptr conversions.
-  virtual bool GetAsList(ListValue** out_value);
-  virtual bool GetAsList(const ListValue** out_value) const;
+  bool GetAsList(ListValue** out_value);
+  bool GetAsList(const ListValue** out_value) const;
   // DictionaryValue::From is the equivalent for std::unique_ptr conversions.
-  virtual bool GetAsDictionary(DictionaryValue** out_value);
-  virtual bool GetAsDictionary(const DictionaryValue** out_value) const;
+  bool GetAsDictionary(DictionaryValue** out_value);
+  bool GetAsDictionary(const DictionaryValue** out_value) const;
   // Note: Do not add more types. See the file-level comment above for why.
 
   // This creates a deep copy of the entire Value tree, and returns a pointer
   // to the copy. The caller gets ownership of the copy, of course.
   // Subclasses return their own type directly in their overrides;
   // this works because C++ supports covariant return types.
-  virtual Value* DeepCopy() const;
+  Value* DeepCopy() const;
   // Preferred version of DeepCopy. TODO(estade): remove the above.
   std::unique_ptr<Value> CreateDeepCopy() const;
 
   // Compares if two Value objects have equal contents.
-  virtual bool Equals(const Value* other) const;
+  bool Equals(const Value* other) const;
 
   // Compares if two Value objects have equal contents. Can handle NULLs.
   // NULLs are considered equal but different from Value::CreateNullValue().
   static bool Equals(const Value* a, const Value* b);
 
- private:
-  void InternalCopyFundamentalValue(const Value& that);
-  void InternalCopyConstructFrom(const Value& that);
-  void InternalMoveConstructFrom(Value&& that);
-  void InternalCopyAssignFrom(const Value& that);
-  void InternalMoveAssignFrom(Value&& that);
-  void InternalCleanup();
-
+ protected:
+  // TODO(crbug.com/646113): Make these private once DictionaryValue and
+  // ListValue are properly inlined.
   Type type_;
 
   union {
@@ -183,7 +181,20 @@ class BASE_EXPORT Value {
     double double_value_;
     ManualConstructor<std::string> string_value_;
     ManualConstructor<std::vector<char>> binary_value_;
+    // For current gcc and clang sizeof(DictStorage) = 48, which would result
+    // in sizeof(Value) = 56 if DictStorage was stack allocated. Allocating it
+    // on the heap results in sizeof(Value) = 40 for all of gcc, clang and MSVC.
+    ManualConstructor<std::unique_ptr<DictStorage>> dict_ptr_;
+    ManualConstructor<ListStorage> list_;
   };
+
+ private:
+  void InternalCopyFundamentalValue(const Value& that);
+  void InternalCopyConstructFrom(const Value& that);
+  void InternalMoveConstructFrom(Value&& that);
+  void InternalCopyAssignFrom(const Value& that);
+  void InternalMoveAssignFrom(Value&& that);
+  void InternalCleanup();
 };
 
 // DictionaryValue provides a key-value dictionary with (optional) "path"
@@ -191,25 +202,19 @@ class BASE_EXPORT Value {
 // are |std::string|s and should be UTF-8 encoded.
 class BASE_EXPORT DictionaryValue : public Value {
  public:
-  using Storage = std::map<std::string, std::unique_ptr<Value>>;
   // Returns |value| if it is a dictionary, nullptr otherwise.
   static std::unique_ptr<DictionaryValue> From(std::unique_ptr<Value> value);
 
   DictionaryValue();
-  ~DictionaryValue() override;

[jdoerrie, 2017/02/15 15:29:09]

Deleting this and the |DISALLOW_COPY_AND_ASSIGN| macro results in copy and move
operations to be implicitly declared and implemented by the compiler. Since they
are doing the right thing I decided to go for it. However, this also means that
they are likely inlined. Given the fact that there are no fields in this class
and |Value|'s copy and move operations are defined out of line (and that this
class will likely be deleted soon) I decided this is fine. What do you think?

[brettw, 2017/02/15 21:28:29]

Your reasoning sounds right to me.

-
-  // Overridden from Value:
-  bool GetAsDictionary(DictionaryValue** out_value) override;
-  bool GetAsDictionary(const DictionaryValue** out_value) const override;
 
   // Returns true if the current dictionary has a value for the given key.
   bool HasKey(StringPiece key) const;
 
   // Returns the number of Values in this dictionary.
-  size_t size() const { return dictionary_.size(); }
+  size_t size() const { return (*dict_ptr_)->size(); }
 
   // Returns whether the dictionary is empty.
-  bool empty() const { return dictionary_.empty(); }
+  bool empty() const { return (*dict_ptr_)->empty(); }
 
   // Clears any current contents of this dictionary.
   void Clear();
@@ -309,8 +314,8 @@ class BASE_EXPORT DictionaryValue : public Value {
 
   // Like Remove(), but without special treatment of '.'.  This allows e.g. URLs
   // to be used as paths.
-  virtual bool RemoveWithoutPathExpansion(StringPiece key,
-                                          std::unique_ptr<Value>* out_value);
+  bool RemoveWithoutPathExpansion(StringPiece key,
+                                  std::unique_ptr<Value>* out_value);
 
   // Removes a path, clearing out all dictionaries on |path| that remain empty
   // after removing the value at |path|.
@@ -328,7 +333,7 @@ class BASE_EXPORT DictionaryValue : public Value {
   void MergeDictionary(const DictionaryValue* dictionary);
 
   // Swaps contents with the |other| dictionary.
-  virtual void Swap(DictionaryValue* other);
+  void Swap(DictionaryValue* other);
 
   // This class provides an iterator over both keys and values in the
   // dictionary.  It can't be used to modify the dictionary.
@@ -338,7 +343,7 @@ class BASE_EXPORT DictionaryValue : public Value {
     Iterator(const Iterator& other);
     ~Iterator();
 
-    bool IsAtEnd() const { return it_ == target_.dictionary_.end(); }
+    bool IsAtEnd() const { return it_ == (*target_.dict_ptr_)->end(); }
     void Advance() { ++it_; }
 
     const std::string& key() const { return it_->first; }
@@ -346,42 +351,33 @@ class BASE_EXPORT DictionaryValue : public Value {
 
    private:
     const DictionaryValue& target_;
-    Storage::const_iterator it_;
+    DictStorage::const_iterator it_;
   };
 
-  // Overridden from Value:
-  DictionaryValue* DeepCopy() const override;
+  DictionaryValue* DeepCopy() const;
   // Preferred version of DeepCopy. TODO(estade): remove the above.
   std::unique_ptr<DictionaryValue> CreateDeepCopy() const;
-  bool Equals(const Value* other) const override;
-
- private:
-  Storage dictionary_;
-
-  DISALLOW_COPY_AND_ASSIGN(DictionaryValue);
 };
 
 // This type of Value represents a list of other Value values.
 class BASE_EXPORT ListValue : public Value {
  public:
-  using Storage = std::vector<std::unique_ptr<Value>>;
-  using const_iterator = Storage::const_iterator;
-  using iterator = Storage::iterator;
+  using const_iterator = ListStorage::const_iterator;
+  using iterator = ListStorage::iterator;
 
   // Returns |value| if it is a list, nullptr otherwise.
   static std::unique_ptr<ListValue> From(std::unique_ptr<Value> value);
 
   ListValue();
-  ~ListValue() override;

[jdoerrie, 2017/02/15 15:29:09]

Same here.

 
   // Clears the contents of this ListValue
   void Clear();
 
   // Returns the number of Values in this list.
-  size_t GetSize() const { return list_.size(); }
+  size_t GetSize() const { return list_->size(); }
 
   // Returns whether the list is empty.
-  bool empty() const { return list_.empty(); }
+  bool empty() const { return list_->empty(); }
 
   // Sets the list item at the given index to be the Value specified by
   // the value given.  If the index beyond the current end of the list, null
@@ -422,7 +418,7 @@ class BASE_EXPORT ListValue : public Value {
   // passed out via |out_value|.  If |out_value| is NULL, the removed value will
   // be deleted.  This method returns true if |index| is valid; otherwise
   // it will return false and the ListValue object will be unchanged.
-  virtual bool Remove(size_t index, std::unique_ptr<Value>* out_value);
+  bool Remove(size_t index, std::unique_ptr<Value>* out_value);
 
   // Removes the first instance of |value| found in the list, if any, and
   // deletes it. |index| is the location where |value| was found. Returns false
@@ -465,28 +461,18 @@ class BASE_EXPORT ListValue : public Value {
   const_iterator Find(const Value& value) const;
 
   // Swaps contents with the |other| list.
-  virtual void Swap(ListValue* other);
+  void Swap(ListValue* other);
 
   // Iteration.
-  iterator begin() { return list_.begin(); }
-  iterator end() { return list_.end(); }
-
-  const_iterator begin() const { return list_.begin(); }
-  const_iterator end() const { return list_.end(); }
+  iterator begin() { return list_->begin(); }
+  iterator end() { return list_->end(); }
 
-  // Overridden from Value:
-  bool GetAsList(ListValue** out_value) override;
-  bool GetAsList(const ListValue** out_value) const override;
-  ListValue* DeepCopy() const override;
-  bool Equals(const Value* other) const override;
+  const_iterator begin() const { return list_->begin(); }
+  const_iterator end() const { return list_->end(); }
 
+  ListValue* DeepCopy() const;
   // Preferred version of DeepCopy. TODO(estade): remove DeepCopy.
   std::unique_ptr<ListValue> CreateDeepCopy() const;
-
- private:
-  Storage list_;
-
-  DISALLOW_COPY_AND_ASSIGN(ListValue);
 };
 
 // This interface is implemented by classes that know how to serialize