Move Storage for ListValue and DictValue in Union

base/values.h

 // 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.
 
 // This file specifies a recursive data storage class called Value intended for
 // storing settings and other persistable data.
 //
 // A Value represents something that can be stored in JSON or passed to/from
 // JavaScript. As such, it is NOT a generalized variant type, since only the
 // types supported by JavaScript/JSON are supported.
@@ skipping 32 matching lines @@
 using StringValue = Value;
 using BinaryValue = Value;
 
 // The Value class is the base class for Values. A Value can be instantiated
 // via the Create*Value() factory methods, or by directly creating instances of
 // the subclasses.
 //
 // See the file-level comment above for more information.
 class BASE_EXPORT Value {
  public:
+  using DictStorage = std::map<std::string, std::unique_ptr<Value>>;

[brettw, 2017/02/11 00:24:04]

As I commented in the private thread, does the size of Value go down if this is
a unique_ptr<std::map<...>>? If so, I think it's worth doing.

[jdoerrie, 2017/02/14 17:06:56]

Done. Wrapping the map in a unique_ptr results in sizeof(Value) == 40 for MSVC,
Clang and GCC.

+  using ListStorage = std::vector<std::unique_ptr<Value>>;
+
   enum class Type {
     NONE = 0,
     BOOLEAN,
     INTEGER,
     DOUBLE,
     STRING,
     BINARY,
     DICTIONARY,
     LIST
     // Note: Do not add more types. See the file-level comment above for why.
@@ skipping 29 matching lines @@
   explicit Value(const char16* in_string);
   explicit Value(const string16& in_string);
   explicit Value(StringPiece in_string);
 
   explicit Value(const std::vector<char>& in_blob);
   explicit Value(std::vector<char>&& in_blob);
 
   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);
 
   // Returns the type of the value stored by the current Value object.
   // Each type will be implemented by only one subclass of Value, so it's
   // safe to use the Type to determine whether you can cast from
   // Value* to (Implementing Class)*.  Also, a Value object never changes
   // its type after construction.
   Type GetType() const { return type_; }  // DEPRECATED, use type().
@@ skipping 16 matching lines @@
   const std::string& GetString() const;
   const std::vector<char>& GetBlob() const;
 
   size_t GetSize() const;         // DEPRECATED, use GetBlob().size() instead.
   const char* GetBuffer() const;  // DEPRECATED, use GetBlob().data() instead.
 
   // These methods allow the convenient retrieval of the contents of the 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 {
     bool bool_value_;
     int int_value_;
     double double_value_;
     ManualConstructor<std::string> string_value_;
     ManualConstructor<std::vector<char>> binary_value_;
+    ManualConstructor<DictStorage> dictionary_;
+    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"
 // parsing for recursive access; see the comment at the top of the file. Keys
 // 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;
-
-  // Overridden from Value:
-  bool GetAsDictionary(DictionaryValue** out_value) override;
-  bool GetAsDictionary(const DictionaryValue** out_value) const override;
+  ~DictionaryValue();
 
   // 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 dictionary_->size(); }
 
   // Returns whether the dictionary is empty.
-  bool empty() const { return dictionary_.empty(); }
+  bool empty() const { return dictionary_->empty(); }
 
   // Clears any current contents of this dictionary.
   void Clear();
 
   // Sets the Value associated with the given path starting from this object.
   // A path has the form "<key>" or "<key>.<key>.[...]", where "." indexes
   // into the next DictionaryValue down.  Obviously, "." can't be used
   // within a key, but there are no other restrictions on keys.
   // If the key at any step of the way doesn't exist, or exists but isn't
   // a DictionaryValue, a new DictionaryValue will be created and attached
@@ skipping 79 matching lines @@
   // Removes the Value with the specified path from this dictionary (or one
   // of its child dictionaries, if the path is more than just a local key).
   // If |out_value| is non-NULL, the removed Value will be passed out via
   // |out_value|.  If |out_value| is NULL, the removed value will be deleted.
   // This method returns true if |path| is a valid path; otherwise it will
   // return false and the DictionaryValue object will be unchanged.
   bool Remove(StringPiece path, std::unique_ptr<Value>* out_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|.
   bool RemovePath(StringPiece path, std::unique_ptr<Value>* out_value);
 
   // Makes a copy of |this| but doesn't include empty dictionaries and lists in
   // the copy.  This never returns NULL, even if |this| itself is empty.
   std::unique_ptr<DictionaryValue> DeepCopyWithoutEmptyChildren() const;
 
   // Merge |dictionary| into this dictionary. This is done recursively, i.e. any
   // sub-dictionaries will be merged as well. In case of key collisions, the
   // passed in dictionary takes precedence and data already present will be
   // replaced. Values within |dictionary| are deep-copied, so |dictionary| may
   // be freed any time after this call.
   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.
   class BASE_EXPORT Iterator {
    public:
     explicit Iterator(const DictionaryValue& target);
     Iterator(const Iterator& other);
     ~Iterator();
 
-    bool IsAtEnd() const { return it_ == target_.dictionary_.end(); }
+    bool IsAtEnd() const { return it_ == target_.dictionary_->end(); }
     void Advance() { ++it_; }
 
     const std::string& key() const { return it_->first; }
     const Value& value() const { return *it_->second; }
 
    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);

[jdoerrie, 2017/02/10 18:45:44]

Given that it is currently impossible to copy construct or assign
DictionaryValues due to the contained unique pointers this probably should have
been kept. Also we should probably implement the move constructor and move
assignment operator here.

 };
 
 // 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;
+  ~ListValue();
 
   // 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
   // Values will be used to pad out the list.
   // Returns true if successful, or false if the index was negative or
   // the value is a null pointer.
   bool Set(size_t index, Value* in_value);
   // Preferred version of the above. TODO(estade): remove the above.
   bool Set(size_t index, std::unique_ptr<Value> in_value);
 
@@ skipping 20 matching lines @@
   bool GetDictionary(size_t index, const DictionaryValue** out_value) const;
   bool GetDictionary(size_t index, DictionaryValue** out_value);
   bool GetList(size_t index, const ListValue** out_value) const;
   bool GetList(size_t index, ListValue** out_value);
 
   // Removes the Value with the specified index from this list.
   // If |out_value| is non-NULL, the removed Value AND ITS OWNERSHIP will be
   // 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
   // if not found.
   bool Remove(const Value& value, size_t* index);
 
   // Removes the element at |iter|. If |out_value| is NULL, the value will be
   // deleted, otherwise ownership of the value is passed back to the caller.
   // Returns an iterator pointing to the location of the element that
   // followed the erased element.
@@ skipping 22 matching lines @@
   // Insert a Value at index.
   // Returns true if successful, or false if the index was out of range.
   bool Insert(size_t index, std::unique_ptr<Value> in_value);
 
   // Searches for the first instance of |value| in the list using the Equals
   // method of the Value type.
   // Returns a const_iterator to the found item or to end() if none exists.
   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(); }
+  iterator begin() { return list_->begin(); }
+  iterator end() { return list_->end(); }
 
-  const_iterator begin() const { return list_.begin(); }
-  const_iterator end() const { return list_.end(); }
+  const_iterator begin() const { return list_->begin(); }
+  const_iterator end() const { 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;
-
+  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);

[jdoerrie, 2017/02/10 18:45:44]

Same as above.

 };
 
 // This interface is implemented by classes that know how to serialize
 // Value objects.
 class BASE_EXPORT ValueSerializer {
  public:
   virtual ~ValueSerializer();
 
   virtual bool Serialize(const Value& root) = 0;
 };
@@ skipping 30 matching lines @@
   return out << static_cast<const Value&>(value);
 }
 
 // Stream operator so that enum class Types can be used in log statements.
 BASE_EXPORT std::ostream& operator<<(std::ostream& out,
                                      const Value::Type& type);
 
 }  // namespace base
 
 #endif  // BASE_VALUES_H_