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

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 11 matching lines @@
 
 namespace {
 
 std::unique_ptr<Value> CopyWithoutEmptyChildren(const Value& node);
 
 // Make a deep copy of |node|, but don't include empty lists or dictionaries
 // in the copy. It's possible for this function to return NULL and it
 // expects |node| to always be non-NULL.
 std::unique_ptr<ListValue> CopyListWithoutEmptyChildren(const ListValue& list) {
   std::unique_ptr<ListValue> copy;
-  for (ListValue::const_iterator it = list.begin(); it != list.end(); ++it) {
-    std::unique_ptr<Value> child_copy = CopyWithoutEmptyChildren(**it);
+  for (const auto& entry : list) {
+    std::unique_ptr<Value> child_copy = CopyWithoutEmptyChildren(*entry);
     if (child_copy) {
       if (!copy)
         copy.reset(new ListValue);
       copy->Append(std::move(child_copy));
     }
   }
   return copy;
 }
 
 std::unique_ptr<DictionaryValue> CopyDictionaryWithoutEmptyChildren(
@@ skipping 17 matching lines @@
 
     case Value::TYPE_DICTIONARY:
       return CopyDictionaryWithoutEmptyChildren(
           static_cast<const DictionaryValue&>(node));
 
     default:
       return node.CreateDeepCopy();
   }
 }
 
-// A small functor for comparing Values for std::find_if and similar.
-class ValueEquals {
- public:
-  // Pass the value against which all consecutive calls of the () operator will
-  // compare their argument to. This Value object must not be destroyed while
-  // the ValueEquals is  in use.
-  explicit ValueEquals(const Value* first) : first_(first) { }
-
-  bool operator ()(const Value* second) const {
-    return first_->Equals(second);
-  }
-
- private:
-  const Value* first_;
-};
-
 }  // namespace
 
 Value::~Value() {
 }
 
 // static
 std::unique_ptr<Value> Value::CreateNullValue() {
   return WrapUnique(new Value(TYPE_NULL));
 }
 
@@ skipping 279 matching lines @@
 }
 
 bool DictionaryValue::GetAsDictionary(const DictionaryValue** out_value) const {
   if (out_value)
     *out_value = this;
   return true;
 }
 
 bool DictionaryValue::HasKey(const std::string& key) const {
   DCHECK(IsStringUTF8(key));
-  ValueMap::const_iterator current_entry = dictionary_.find(key);
+  auto current_entry = dictionary_.find(key);
   DCHECK((current_entry == dictionary_.end()) || current_entry->second);
   return current_entry != dictionary_.end();
 }
 
 void DictionaryValue::Clear() {
-  ValueMap::iterator dict_iterator = dictionary_.begin();
-  while (dict_iterator != dictionary_.end()) {
-    delete dict_iterator->second;
-    ++dict_iterator;
-  }
-
   dictionary_.clear();
 }
 
 void DictionaryValue::Set(const std::string& path,
                           std::unique_ptr<Value> in_value) {
   DCHECK(IsStringUTF8(path));
   DCHECK(in_value);
 
   std::string current_path(path);
   DictionaryValue* current_dictionary = this;
@@ skipping 37 matching lines @@
   Set(path, new StringValue(in_value));
 }
 
 void DictionaryValue::SetString(const std::string& path,
                                 const string16& in_value) {
   Set(path, new StringValue(in_value));
 }
 
 void DictionaryValue::SetWithoutPathExpansion(const std::string& key,
                                               std::unique_ptr<Value> in_value) {
-  Value* bare_ptr = in_value.release();
-  // If there's an existing value here, we need to delete it, because
-  // we own all our children.
-  std::pair<ValueMap::iterator, bool> ins_res =
-      dictionary_.insert(std::make_pair(key, bare_ptr));
-  if (!ins_res.second) {
-    DCHECK_NE(ins_res.first->second, bare_ptr);  // This would be bogus
-    delete ins_res.first->second;
-    ins_res.first->second = bare_ptr;
-  }
+  dictionary_[key] = std::move(in_value);
 }
 
 void DictionaryValue::SetWithoutPathExpansion(const std::string& key,
                                               Value* in_value) {
   SetWithoutPathExpansion(key, WrapUnique(in_value));
 }
 
 void DictionaryValue::SetBooleanWithoutPathExpansion(
     const std::string& path, bool in_value) {
   SetWithoutPathExpansion(path, new FundamentalValue(in_value));
@@ skipping 163 matching lines @@
 
 bool DictionaryValue::GetList(const std::string& path, ListValue** out_value) {
   return static_cast<const DictionaryValue&>(*this).GetList(
       path,
       const_cast<const ListValue**>(out_value));
 }
 
 bool DictionaryValue::GetWithoutPathExpansion(const std::string& key,
                                               const Value** out_value) const {
   DCHECK(IsStringUTF8(key));
-  ValueMap::const_iterator entry_iterator = dictionary_.find(key);
+  auto entry_iterator = dictionary_.find(key);
   if (entry_iterator == dictionary_.end())
     return false;
 
-  const Value* entry = entry_iterator->second;
   if (out_value)
-    *out_value = entry;
+    *out_value = entry_iterator->second.get();
   return true;
 }
 
 bool DictionaryValue::GetWithoutPathExpansion(const std::string& key,
                                               Value** out_value) {
   return static_cast<const DictionaryValue&>(*this).GetWithoutPathExpansion(
       key,
       const_cast<const Value**>(out_value));
 }
 
@@ skipping 103 matching lines @@
   }
 
   return current_dictionary->RemoveWithoutPathExpansion(current_path,
                                                         out_value);
 }
 
 bool DictionaryValue::RemoveWithoutPathExpansion(
     const std::string& key,
     std::unique_ptr<Value>* out_value) {
   DCHECK(IsStringUTF8(key));
-  ValueMap::iterator entry_iterator = dictionary_.find(key);
+  auto entry_iterator = dictionary_.find(key);
   if (entry_iterator == dictionary_.end())
     return false;
 
-  Value* entry = entry_iterator->second;
   if (out_value)
-    out_value->reset(entry);
-  else
-    delete entry;
+    *out_value = std::move(entry_iterator->second);
   dictionary_.erase(entry_iterator);
   return true;
 }
 
 bool DictionaryValue::RemovePath(const std::string& path,
                                  std::unique_ptr<Value>* out_value) {
   bool result = false;
   size_t delimiter_position = path.find('.');
 
   if (delimiter_position == std::string::npos)
@@ skipping 45 matching lines @@
     : target_(target),
       it_(target.dictionary_.begin()) {}
 
 DictionaryValue::Iterator::Iterator(const Iterator& other) = default;
 
 DictionaryValue::Iterator::~Iterator() {}
 
 DictionaryValue* DictionaryValue::DeepCopy() const {
   DictionaryValue* result = new DictionaryValue;
 
-  for (ValueMap::const_iterator current_entry(dictionary_.begin());
-       current_entry != dictionary_.end(); ++current_entry) {
-    result->SetWithoutPathExpansion(current_entry->first,
-                                    current_entry->second->DeepCopy());
+  for (const auto& current_entry : dictionary_) {
+    result->SetWithoutPathExpansion(current_entry.first,
+                                    current_entry.second->CreateDeepCopy());
   }
 
   return result;
 }
 
 std::unique_ptr<DictionaryValue> DictionaryValue::CreateDeepCopy() const {
   return WrapUnique(DeepCopy());
 }
 
 bool DictionaryValue::Equals(const Value* other) const {
@@ skipping 31 matching lines @@
 }
 
 ListValue::ListValue() : Value(TYPE_LIST) {
 }
 
 ListValue::~ListValue() {
   Clear();
 }
 
 void ListValue::Clear() {
-  for (ValueVector::iterator i(list_.begin()); i != list_.end(); ++i)
-    delete *i;
   list_.clear();
 }
 
 bool ListValue::Set(size_t index, Value* in_value) {
+  return Set(index, base::WrapUnique(in_value));

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

I changed a few of the obvious things I touched so that the std::unique_ptr
version is now the canonical version.

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

nit: no base:: in base

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

Done.

+}
+
+bool ListValue::Set(size_t index, std::unique_ptr<Value> in_value) {
   if (!in_value)
     return false;
 
   if (index >= list_.size()) {
     // Pad out any intermediate indexes with null settings
     while (index > list_.size())
       Append(CreateNullValue());
-    Append(in_value);
+    Append(std::move(in_value));
   } else {
+    // TODO(dcheng): remove this DCHECK once the raw pointer version is removed?
     DCHECK(list_[index] != in_value);
-    delete list_[index];
-    list_[index] = in_value;
+    list_[index] = std::move(in_value);
   }
   return true;
 }
 
-bool ListValue::Set(size_t index, std::unique_ptr<Value> in_value) {
-  return Set(index, in_value.release());
-}
-
 bool ListValue::Get(size_t index, const Value** out_value) const {
   if (index >= list_.size())
     return false;
 
   if (out_value)
-    *out_value = list_[index];
+    *out_value = list_[index].get();
 
   return true;
 }
 
 bool ListValue::Get(size_t index, Value** out_value) {
   return static_cast<const ListValue&>(*this).Get(
       index,
       const_cast<const Value**>(out_value));
 }
 
@@ skipping 90 matching lines @@
   return static_cast<const ListValue&>(*this).GetList(
       index,
       const_cast<const ListValue**>(out_value));
 }
 
 bool ListValue::Remove(size_t index, std::unique_ptr<Value>* out_value) {
   if (index >= list_.size())
     return false;
 
   if (out_value)
-    out_value->reset(list_[index]);
-  else
-    delete list_[index];
+    *out_value = std::move(list_[index]);
 
   list_.erase(list_.begin() + index);
   return true;
 }
 
 bool ListValue::Remove(const Value& value, size_t* index) {
-  for (ValueVector::iterator i(list_.begin()); i != list_.end(); ++i) {
-    if ((*i)->Equals(&value)) {
-      size_t previous_index = i - list_.begin();
-      delete *i;
-      list_.erase(i);
+  for (auto it = list_.begin(); it != list_.end(); ++it) {
+    if ((*it)->Equals(&value)) {
+      size_t previous_index = it - list_.begin();
+      list_.erase(it);
 
       if (index)
         *index = previous_index;
       return true;
     }
   }
   return false;
 }
 
 ListValue::iterator ListValue::Erase(iterator iter,
                                      std::unique_ptr<Value>* out_value) {
   if (out_value)
     out_value->reset(*iter);
   else
     delete *iter;
 
   return list_.erase(iter);
 }
 
 void ListValue::Append(std::unique_ptr<Value> in_value) {
-  Append(in_value.release());
+  list_.emplace_back(std::move(in_value));
 }
 
 void ListValue::Append(Value* in_value) {
   DCHECK(in_value);
-  list_.push_back(in_value);
+  Append(base::WrapUnique(in_value));
 }
 
 void ListValue::AppendBoolean(bool in_value) {
   Append(new FundamentalValue(in_value));
 }
 
 void ListValue::AppendInteger(int in_value) {
   Append(new FundamentalValue(in_value));
 }
 
@@ skipping 18 matching lines @@
 
 void ListValue::AppendStrings(const std::vector<string16>& in_values) {
   for (std::vector<string16>::const_iterator it = in_values.begin();
        it != in_values.end(); ++it) {
     AppendString(*it);
   }
 }
 
 bool ListValue::AppendIfNotPresent(Value* in_value) {
   DCHECK(in_value);
-  for (ValueVector::const_iterator i(list_.begin()); i != list_.end(); ++i) {
-    if ((*i)->Equals(in_value)) {
+  for (const auto& entry : list_) {
+    if (entry->Equals(in_value)) {
       delete in_value;
       return false;
     }
   }
-  list_.push_back(in_value);
+  list_.emplace_back(in_value);
   return true;
 }
 
 bool ListValue::Insert(size_t index, Value* in_value) {
   DCHECK(in_value);
   if (index > list_.size())
     return false;
 
-  list_.insert(list_.begin() + index, in_value);
+  list_.insert(list_.begin() + index, base::WrapUnique(in_value));

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

nit -base::

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

Done.

   return true;
 }
 
 ListValue::const_iterator ListValue::Find(const Value& value) const {
-  return std::find_if(list_.begin(), list_.end(), ValueEquals(&value));
+  return std::find_if(list_.begin(), list_.end(),
+                      [&value](const std::unique_ptr<Value>& entry) {
+                        return entry->Equals(&value);
+                      });
 }
 
 void ListValue::Swap(ListValue* other) {
   list_.swap(other->list_);
 }
 
 bool ListValue::GetAsList(ListValue** out_value) {
   if (out_value)
     *out_value = this;
   return true;
 }
 
 bool ListValue::GetAsList(const ListValue** out_value) const {
   if (out_value)
     *out_value = this;
   return true;
 }
 
 ListValue* ListValue::DeepCopy() const {
   ListValue* result = new ListValue;
 
-  for (ValueVector::const_iterator i(list_.begin()); i != list_.end(); ++i)
-    result->Append((*i)->DeepCopy());
+  for (const auto& entry : list_)
+    result->Append(entry->CreateDeepCopy());
 
   return result;
 }
 
 std::unique_ptr<ListValue> ListValue::CreateDeepCopy() const {
   return WrapUnique(DeepCopy());
 }
 
 bool ListValue::Equals(const Value* other) const {
   if (other->GetType() != GetType())
     return false;
 
   const ListValue* other_list =
       static_cast<const ListValue*>(other);
-  const_iterator lhs_it, rhs_it;
+  Storage::const_iterator lhs_it, rhs_it;
   for (lhs_it = begin(), rhs_it = other_list->begin();
        lhs_it != end() && rhs_it != other_list->end();
        ++lhs_it, ++rhs_it) {
-    if (!(*lhs_it)->Equals(*rhs_it))
+    if (!(*lhs_it)->Equals(&**rhs_it))

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

lol.. how about using &rhs_it->get() idk if that's more obvious wdyt?

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

Hmm. I have no idea why I thought this was a useful thing to write. Simplified
this considerably.

       return false;
   }
   if (lhs_it != end() || rhs_it != other_list->end())
     return false;
 
   return true;
 }
 
 ValueSerializer::~ValueSerializer() {
 }
 
 ValueDeserializer::~ValueDeserializer() {
 }
 
 std::ostream& operator<<(std::ostream& out, const Value& value) {
   std::string json;
   JSONWriter::WriteWithOptions(value, JSONWriter::OPTIONS_PRETTY_PRINT, &json);
   return out << json;
 }
 
 }  // namespace base