Make element removal methods in DictionaryValue and ListValue take scoped_ptr's as outparams.
TBR=pneubeck@chromium.org,scottbyer@chromium.org
BUG=263894
Review URL: https://chromiumcodereview.appspot.com/21030009
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@215885 0039d316-1c4b-4281-b951-d872f2087c98
CrOS-Libchrome-Original-Commit: d814a88597a01bf8249e36983f55a5f880154642
diff --git a/base/debug/trace_event_unittest.cc b/base/debug/trace_event_unittest.cc
index 5beea37..ea295c0 100644
--- a/base/debug/trace_event_unittest.cc
+++ b/base/debug/trace_event_unittest.cc
@@ -149,9 +149,9 @@
// Move items into our aggregate collection
while (root_list->GetSize()) {
- Value* item = NULL;
+ scoped_ptr<Value> item;
root_list->Remove(0, &item);
- trace_parsed_.Append(item);
+ trace_parsed_.Append(item.release());
}
}
diff --git a/base/json/json_parser.cc b/base/json/json_parser.cc
index 4eb9e9a..f1f4333 100644
--- a/base/json/json_parser.cc
+++ b/base/json/json_parser.cc
@@ -56,7 +56,7 @@
// the method below.
virtual bool RemoveWithoutPathExpansion(const std::string& key,
- Value** out) OVERRIDE {
+ scoped_ptr<Value>* out) OVERRIDE {
// If the caller won't take ownership of the removed value, just call up.
if (!out)
return DictionaryValue::RemoveWithoutPathExpansion(key, out);
@@ -65,12 +65,11 @@
// Otherwise, remove the value while its still "owned" by this and copy it
// to convert any JSONStringValues to std::string.
- Value* out_owned = NULL;
+ scoped_ptr<Value> out_owned;
if (!DictionaryValue::RemoveWithoutPathExpansion(key, &out_owned))
return false;
- *out = out_owned->DeepCopy();
- delete out_owned;
+ out->reset(out_owned->DeepCopy());
return true;
}
@@ -103,7 +102,7 @@
ListValue::Swap(copy.get());
}
- virtual bool Remove(size_t index, Value** out) OVERRIDE {
+ virtual bool Remove(size_t index, scoped_ptr<Value>* out) OVERRIDE {
// If the caller won't take ownership of the removed value, just call up.
if (!out)
return ListValue::Remove(index, out);
@@ -112,12 +111,11 @@
// Otherwise, remove the value while its still "owned" by this and copy it
// to convert any JSONStringValues to std::string.
- Value* out_owned = NULL;
+ scoped_ptr<Value> out_owned;
if (!ListValue::Remove(index, &out_owned))
return false;
- *out = out_owned->DeepCopy();
- delete out_owned;
+ out->reset(out_owned->DeepCopy());
return true;
}
diff --git a/base/json/json_reader_unittest.cc b/base/json/json_reader_unittest.cc
index 41650b4..527cb97 100644
--- a/base/json/json_reader_unittest.cc
+++ b/base/json/json_reader_unittest.cc
@@ -559,9 +559,12 @@
// Tests that the root of a JSON object can be deleted safely while its
// children outlive it.
TEST(JSONReaderTest, StringOptimizations) {
- Value* dict_literals[2] = {0};
- Value* dict_strings[2] = {0};
- Value* list_values[2] = {0};
+ scoped_ptr<Value> dict_literal_0;
+ scoped_ptr<Value> dict_literal_1;
+ scoped_ptr<Value> dict_string_0;
+ scoped_ptr<Value> dict_string_1;
+ scoped_ptr<Value> list_value_0;
+ scoped_ptr<Value> list_value_1;
{
scoped_ptr<Value> root(JSONReader::Read(
@@ -588,43 +591,36 @@
ASSERT_TRUE(root_dict->GetDictionary("test", &dict));
ASSERT_TRUE(root_dict->GetList("list", &list));
- EXPECT_TRUE(dict->Remove("foo", &dict_literals[0]));
- EXPECT_TRUE(dict->Remove("bar", &dict_literals[1]));
- EXPECT_TRUE(dict->Remove("baz", &dict_strings[0]));
- EXPECT_TRUE(dict->Remove("moo", &dict_strings[1]));
+ EXPECT_TRUE(dict->Remove("foo", &dict_literal_0));
+ EXPECT_TRUE(dict->Remove("bar", &dict_literal_1));
+ EXPECT_TRUE(dict->Remove("baz", &dict_string_0));
+ EXPECT_TRUE(dict->Remove("moo", &dict_string_1));
ASSERT_EQ(2u, list->GetSize());
- EXPECT_TRUE(list->Remove(0, &list_values[0]));
- EXPECT_TRUE(list->Remove(0, &list_values[1]));
+ EXPECT_TRUE(list->Remove(0, &list_value_0));
+ EXPECT_TRUE(list->Remove(0, &list_value_1));
}
bool b = false;
double d = 0;
std::string s;
- EXPECT_TRUE(dict_literals[0]->GetAsBoolean(&b));
+ EXPECT_TRUE(dict_literal_0->GetAsBoolean(&b));
EXPECT_TRUE(b);
- EXPECT_TRUE(dict_literals[1]->GetAsDouble(&d));
+ EXPECT_TRUE(dict_literal_1->GetAsDouble(&d));
EXPECT_EQ(3.14, d);
- EXPECT_TRUE(dict_strings[0]->GetAsString(&s));
+ EXPECT_TRUE(dict_string_0->GetAsString(&s));
EXPECT_EQ("bat", s);
- EXPECT_TRUE(dict_strings[1]->GetAsString(&s));
+ EXPECT_TRUE(dict_string_1->GetAsString(&s));
EXPECT_EQ("cow", s);
- EXPECT_TRUE(list_values[0]->GetAsString(&s));
+ EXPECT_TRUE(list_value_0->GetAsString(&s));
EXPECT_EQ("a", s);
- EXPECT_TRUE(list_values[1]->GetAsString(&s));
+ EXPECT_TRUE(list_value_1->GetAsString(&s));
EXPECT_EQ("b", s);
-
- delete dict_literals[0];
- delete dict_literals[1];
- delete dict_strings[0];
- delete dict_strings[1];
- delete list_values[0];
- delete list_values[1];
}
// A smattering of invalid JSON designed to test specific portions of the
diff --git a/base/values.cc b/base/values.cc
index 712fff3..adfb980 100644
--- a/base/values.cc
+++ b/base/values.cc
@@ -719,7 +719,8 @@
const_cast<const ListValue**>(out_value));
}
-bool DictionaryValue::Remove(const std::string& path, Value** out_value) {
+bool DictionaryValue::Remove(const std::string& path,
+ scoped_ptr<Value>* out_value) {
DCHECK(IsStringUTF8(path));
std::string current_path(path);
DictionaryValue* current_dictionary = this;
@@ -736,7 +737,7 @@
}
bool DictionaryValue::RemoveWithoutPathExpansion(const std::string& key,
- Value** out_value) {
+ scoped_ptr<Value>* out_value) {
DCHECK(IsStringUTF8(key));
ValueMap::iterator entry_iterator = dictionary_.find(key);
if (entry_iterator == dictionary_.end())
@@ -744,7 +745,7 @@
Value* entry = entry_iterator->second;
if (out_value)
- *out_value = entry;
+ out_value->reset(entry);
else
delete entry;
dictionary_.erase(entry_iterator);
@@ -958,12 +959,12 @@
const_cast<const ListValue**>(out_value));
}
-bool ListValue::Remove(size_t index, Value** out_value) {
+bool ListValue::Remove(size_t index, scoped_ptr<Value>* out_value) {
if (index >= list_.size())
return false;
if (out_value)
- *out_value = list_[index];
+ out_value->reset(list_[index]);
else
delete list_[index];
@@ -986,9 +987,10 @@
return false;
}
-ListValue::iterator ListValue::Erase(iterator iter, Value** out_value) {
+ListValue::iterator ListValue::Erase(iterator iter,
+ scoped_ptr<Value>* out_value) {
if (out_value)
- *out_value = *iter;
+ out_value->reset(*iter);
else
delete *iter;
diff --git a/base/values.h b/base/values.h
index 06e9458..4025c75 100644
--- a/base/values.h
+++ b/base/values.h
@@ -311,16 +311,16 @@
// 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 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 |path| is a valid path; otherwise
- // it will return false and the DictionaryValue object will be unchanged.
- virtual bool Remove(const std::string& path, Value** out_value);
+ // 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.
+ virtual bool Remove(const std::string& path, scoped_ptr<Value>* out_value);
// Like Remove(), but without special treatment of '.'. This allows e.g. URLs
// to be used as paths.
virtual bool RemoveWithoutPathExpansion(const std::string& key,
- Value** out_value);
+ scoped_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.
@@ -414,7 +414,7 @@
// 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, Value** out_value);
+ virtual bool Remove(size_t index, scoped_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
@@ -425,7 +425,7 @@
// 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.
- iterator Erase(iterator iter, Value** out_value);
+ iterator Erase(iterator iter, scoped_ptr<Value>* out_value);
// Appends a Value to the end of the list.
void Append(Value* in_value);
diff --git a/base/values_unittest.cc b/base/values_unittest.cc
index 8776c10..733c485 100644
--- a/base/values_unittest.cc
+++ b/base/values_unittest.cc
@@ -203,7 +203,7 @@
TEST(ValuesTest, ListRemoval) {
bool deletion_flag = true;
- Value* removed_item = NULL;
+ scoped_ptr<Value> removed_item;
{
ListValue list;
@@ -218,8 +218,7 @@
EXPECT_EQ(0U, list.GetSize());
}
EXPECT_FALSE(deletion_flag);
- delete removed_item;
- removed_item = NULL;
+ removed_item.reset();
EXPECT_TRUE(deletion_flag);
{
@@ -275,7 +274,7 @@
TEST(ValuesTest, DictionaryRemoval) {
std::string key = "test";
bool deletion_flag = true;
- Value* removed_item = NULL;
+ scoped_ptr<Value> removed_item;
{
DictionaryValue dict;
@@ -288,8 +287,7 @@
ASSERT_TRUE(removed_item);
}
EXPECT_FALSE(deletion_flag);
- delete removed_item;
- removed_item = NULL;
+ removed_item.reset();
EXPECT_TRUE(deletion_flag);
{