Use std::unique_ptr for base::DictionaryValue and base::ListValue's internal store.

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 24 matching lines @@
 
 namespace base {
 
 class BinaryValue;
 class DictionaryValue;
 class FundamentalValue;
 class ListValue;
 class StringValue;
 class Value;
 
-typedef std::vector<Value*> ValueVector;
-typedef std::map<std::string, Value*> ValueMap;
-
 // 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:
   enum Type {
     TYPE_NULL = 0,
     TYPE_BOOLEAN,
@@ skipping 145 matching lines @@
   size_t size_;
 
   DISALLOW_COPY_AND_ASSIGN(BinaryValue);
 };
 
 // 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;
 
@@ skipping 142 matching lines @@
     ~Iterator();
 
     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_;
-    ValueMap::const_iterator it_;
+    Storage::const_iterator it_;
   };
 
   // Overridden from Value:
   DictionaryValue* DeepCopy() const override;
   // Preferred version of DeepCopy. TODO(estade): remove the above.
   std::unique_ptr<DictionaryValue> CreateDeepCopy() const;
   bool Equals(const Value* other) const override;
 
  private:
-  ValueMap dictionary_;
+  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:
-  typedef ValueVector::iterator iterator;
-  typedef ValueVector::const_iterator const_iterator;
+  using Storage = std::vector<std::unique_ptr<Value>>;
+  class iterator : public Storage::iterator {

[dcheng, 2016/05/20 19:49:47]

There's ~160 dependencies on iterator wrapping a raw pointer right now, which
makes it a little annoying to change ListValue and DictionaryValue to hold
std::unique_ptr<Value>'s internally rather than raw Value pointers.

This is a shim so the external consumers still iterate over the raw pointer
version. The use of std::unique_ptr internally feels like an implementation
detail, so it might be nice to avoid exposing it.

But even if we do want to expose it, it's probably best to separate it into a
separate patch given the number of changes.

[danakj, 2016/05/20 20:51:38]

The subclassing part is weird. It'd be better if you made it a member I think
and just expose the API you want to expose.

[dcheng, 2016/05/20 22:41:55]

I could do that, but it makes it a bit more painful. I'll have to double up the
implementations of:
- operator++
- operator*
- operator->
- operator==
- operator!=
- in addition, operator== and operator!= need to be explicitly made comparable
now.
- the iterator conversion constructor and the iterator copy constructor both
need to be out-of-lined since the clang plugin will now complain
- for STL friendliness, both should derive from
std::iterator<std::forward_iterator_tag, Value> as well.

Finally, I need to expose a way to poke at the internal iterator for code in
values.cc, but this is going to be publicly visible outside base::Value anyway.

In short, there's quite a bit of overhead from not subclassing and it actually
gets a bit harder to read.

With the current subclassing, it's pretty straightforward: we want the exact
same behavior as the wrapped iterator, except for unary operator*.

+   public:
+    iterator(const Storage::iterator& it) : Storage::iterator(it) {}
+    Value* operator*() { return Storage::iterator::operator*().get(); }
+  };
+  class const_iterator : public Storage::const_iterator {
+   public:
+    const_iterator(const Storage::const_iterator& it)
+        : Storage::const_iterator(it) {}
+    const Value* operator*() {

[dcheng, 2016/05/20 19:49:47]

I believe const_iterator just returns Value* here. I could make it match.

+      return Storage::const_iterator::operator*().get();
+    }
+  };
 
   // Returns |value| if it is a list, nullptr otherwise.
   static std::unique_ptr<ListValue> From(std::unique_ptr<Value> value);
 
   ListValue();
   ~ListValue() override;
 
   // Clears the contents of this ListValue
   void Clear();
 
@@ skipping 96 matching lines @@
   // 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;
 
   // Preferred version of DeepCopy. TODO(estade): remove DeepCopy.
   std::unique_ptr<ListValue> CreateDeepCopy() const;
 
  private:
-  ValueVector list_;
+  Storage list_;
 
   DISALLOW_COPY_AND_ASSIGN(ListValue);
 };
 
 // This interface is implemented by classes that know how to serialize
 // Value objects.
 class BASE_EXPORT ValueSerializer {
  public:
   virtual ~ValueSerializer();
 
@@ skipping 38 matching lines @@
 }
 
 BASE_EXPORT inline std::ostream& operator<<(std::ostream& out,
                                             const ListValue& value) {
   return out << static_cast<const Value&>(value);
 }
 
 }  // namespace base
 
 #endif  // BASE_VALUES_H_