Reland of Change base::Value::ListStorage to std::vector<base::Value>
The compilation on ChromeOS failed for the original CL
(http://crrev.com/2811673002), it is likely that a simple rebase fixes the
issue.
This patchset is equivalent to issue 2740143002 at patchset 14, simply another
rebase took place.
Original description follows:
This CL is a first step to inlining base::ListValue. It is proposed to use an
std::vector<base::Value> as the underlying ListStorage. This CL implements the
change and updates the code accordingly.
BUG=646113
TBR=brettw@chromium.org,rdevlin.cronin@chromium.org,flackr@chromium.org,skym@chromium.org,rsesek@chromium.org,bajones@chromium.org,dbeam@chromium.org,stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2809023003
Cr-Commit-Position: refs/heads/master@{#463684}
CrOS-Libchrome-Original-Commit: a5676c607e107238b2d0cdd55e626c93669a92f1
diff --git a/base/json/json_writer.cc b/base/json/json_writer.cc
index 07b9d50..19f28f2 100644
--- a/base/json/json_writer.cc
+++ b/base/json/json_writer.cc
@@ -128,7 +128,7 @@
bool result = node.GetAsList(&list);
DCHECK(result);
for (const auto& value : *list) {
- if (omit_binary_values_ && value->GetType() == Value::Type::BINARY)
+ if (omit_binary_values_ && value.GetType() == Value::Type::BINARY)
continue;
if (first_value_has_been_output) {
@@ -137,7 +137,7 @@
json_string_->push_back(' ');
}
- if (!BuildJSONString(*value, depth))
+ if (!BuildJSONString(value, depth))
result = false;
first_value_has_been_output = true;
diff --git a/base/test/gtest_util.cc b/base/test/gtest_util.cc
index 6da902d..1552c1a 100644
--- a/base/test/gtest_util.cc
+++ b/base/test/gtest_util.cc
@@ -85,7 +85,7 @@
std::vector<base::TestIdentifier> result;
for (base::ListValue::iterator i = tests->begin(); i != tests->end(); ++i) {
base::DictionaryValue* test = nullptr;
- if (!(*i)->GetAsDictionary(&test))
+ if (!i->GetAsDictionary(&test))
return false;
TestIdentifier test_data;
diff --git a/base/trace_event/trace_event_argument.cc b/base/trace_event/trace_event_argument.cc
index db702b6..646b1f2 100644
--- a/base/trace_event/trace_event_argument.cc
+++ b/base/trace_event/trace_event_argument.cc
@@ -289,7 +289,7 @@
value.GetAsList(&list_value);
BeginArrayWithCopiedName(name);
for (const auto& base_value : *list_value)
- AppendBaseValue(*base_value);
+ AppendBaseValue(base_value);
EndArray();
} break;
}
@@ -343,7 +343,7 @@
value.GetAsList(&list_value);
BeginArray();
for (const auto& base_value : *list_value)
- AppendBaseValue(*base_value);
+ AppendBaseValue(base_value);
EndArray();
} break;
}
@@ -369,9 +369,11 @@
cur_dict = new_dict;
} else {
cur_list->Append(WrapUnique(new_dict));
+ // |new_dict| is invalidated at this point, so |cur_dict| needs to be
+ // reset.
+ cur_list->GetDictionary(cur_list->GetSize() - 1, &cur_dict);
stack.push_back(cur_list);
cur_list = nullptr;
- cur_dict = new_dict;
}
} break;
@@ -396,7 +398,8 @@
} else {
cur_list->Append(WrapUnique(new_list));
stack.push_back(cur_list);
- cur_list = new_list;
+ // |cur_list| is invalidated at this point, so it needs to be reset.
+ cur_list->GetList(cur_list->GetSize() - 1, &cur_list);
}
} break;
diff --git a/base/trace_event/trace_event_memory_overhead.cc b/base/trace_event/trace_event_memory_overhead.cc
index ffc4c08..99f0240 100644
--- a/base/trace_event/trace_event_memory_overhead.cc
+++ b/base/trace_event/trace_event_memory_overhead.cc
@@ -105,7 +105,7 @@
value.GetAsList(&list_value);
Add("ListValue", sizeof(ListValue));
for (const auto& v : *list_value)
- AddValue(*v);
+ AddValue(v);
} break;
default:
diff --git a/base/values.cc b/base/values.cc
index ca12edc..74e83b9 100644
--- a/base/values.cc
+++ b/base/values.cc
@@ -35,7 +35,7 @@
std::unique_ptr<ListValue> CopyListWithoutEmptyChildren(const ListValue& list) {
std::unique_ptr<ListValue> copy;
for (const auto& entry : list) {
- std::unique_ptr<Value> child_copy = CopyWithoutEmptyChildren(*entry);
+ std::unique_ptr<Value> child_copy = CopyWithoutEmptyChildren(entry);
if (child_copy) {
if (!copy)
copy.reset(new ListValue);
@@ -375,12 +375,7 @@
std::tie(v.first, *v.second);
});
case Value::Type::LIST:
- if (lhs.list_->size() != rhs.list_->size())
- return false;
- return std::equal(
- std::begin(*lhs.list_), std::end(*lhs.list_), std::begin(*rhs.list_),
- [](const Value::ListStorage::value_type& u,
- const Value::ListStorage::value_type& v) { return *u == *v; });
+ return *lhs.list_ == *rhs.list_;
}
NOTREACHED();
@@ -419,11 +414,7 @@
return std::tie(u.first, *u.second) < std::tie(v.first, *v.second);
});
case Value::Type::LIST:
- return std::lexicographical_compare(
- std::begin(*lhs.list_), std::end(*lhs.list_), std::begin(*rhs.list_),
- std::end(*rhs.list_),
- [](const Value::ListStorage::value_type& u,
- const Value::ListStorage::value_type& v) { return *u < *v; });
+ return *lhs.list_ < *rhs.list_;
}
NOTREACHED();
@@ -494,11 +485,10 @@
case Type::BINARY:
binary_value_.Init(*that.binary_value_);
return;
- // DictStorage and ListStorage are move-only types due to the presence of
- // unique_ptrs. This is why the explicit copy of every element is necessary
- // here.
- // TODO(crbug.com/646113): Clean this up when DictStorage and ListStorage
- // can be copied directly.
+ // DictStorage is a move-only type due to the presence of unique_ptrs. This
+ // is why the explicit copy of every element is necessary here.
+ // TODO(crbug.com/646113): Clean this up when DictStorage can be copied
+ // directly.
case Type::DICTIONARY:
dict_ptr_.Init(MakeUnique<DictStorage>());
for (const auto& it : **that.dict_ptr_) {
@@ -508,10 +498,7 @@
}
return;
case Type::LIST:
- list_.Init();
- list_->reserve(that.list_->size());
- for (const auto& it : *that.list_)
- list_->push_back(MakeUnique<Value>(*it));
+ list_.Init(*that.list_);
return;
}
}
@@ -561,16 +548,17 @@
case Type::BINARY:
*binary_value_ = *that.binary_value_;
return;
- // DictStorage and ListStorage are move-only types due to the presence of
- // unique_ptrs. This is why the explicit call to the copy constructor is
- // necessary here.
- // TODO(crbug.com/646113): Clean this up when DictStorage and ListStorage
- // can be copied directly.
- case Type::DICTIONARY:
- *dict_ptr_ = std::move(*Value(that).dict_ptr_);
+ // DictStorage is a move-only type due to the presence of unique_ptrs. This
+ // is why the explicit call to the copy constructor is necessary here.
+ // TODO(crbug.com/646113): Clean this up when DictStorage can be copied
+ // directly.
+ case Type::DICTIONARY: {
+ Value copy = that;
+ *dict_ptr_ = std::move(*copy.dict_ptr_);
return;
+ }
case Type::LIST:
- *list_ = std::move(*Value(that).list_);
+ *list_ = *that.list_;
return;
}
}
@@ -1077,6 +1065,10 @@
list_->clear();
}
+void ListValue::Reserve(size_t n) {
+ list_->reserve(n);
+}
+
bool ListValue::Set(size_t index, Value* in_value) {
return Set(index, WrapUnique(in_value));
}
@@ -1091,9 +1083,7 @@
Append(MakeUnique<Value>());
Append(std::move(in_value));
} else {
- // TODO(dcheng): remove this DCHECK once the raw pointer version is removed?
- DCHECK((*list_)[index] != in_value);
- (*list_)[index] = std::move(in_value);
+ (*list_)[index] = std::move(*in_value);
}
return true;
}
@@ -1103,7 +1093,7 @@
return false;
if (out_value)
- *out_value = (*list_)[index].get();
+ *out_value = &(*list_)[index];
return true;
}
@@ -1213,36 +1203,35 @@
return false;
if (out_value)
- *out_value = std::move((*list_)[index]);
+ *out_value = MakeUnique<Value>(std::move((*list_)[index]));
list_->erase(list_->begin() + index);
return true;
}
bool ListValue::Remove(const Value& value, size_t* index) {
- for (auto it = list_->begin(); it != list_->end(); ++it) {
- if (**it == value) {
- size_t previous_index = it - list_->begin();
- list_->erase(it);
+ auto it = std::find(list_->begin(), list_->end(), value);
- if (index)
- *index = previous_index;
- return true;
- }
- }
- return false;
+ if (it == list_->end())
+ return false;
+
+ if (index)
+ *index = std::distance(list_->begin(), it);
+
+ list_->erase(it);
+ return true;
}
ListValue::iterator ListValue::Erase(iterator iter,
std::unique_ptr<Value>* out_value) {
if (out_value)
- *out_value = std::move(*ListStorage::iterator(iter));
+ *out_value = MakeUnique<Value>(std::move(*iter));
return list_->erase(iter);
}
void ListValue::Append(std::unique_ptr<Value> in_value) {
- list_->push_back(std::move(in_value));
+ list_->push_back(std::move(*in_value));
}
#if !defined(OS_LINUX)
@@ -1288,11 +1277,10 @@
bool ListValue::AppendIfNotPresent(std::unique_ptr<Value> in_value) {
DCHECK(in_value);
- for (const auto& entry : *list_) {
- if (*entry == *in_value)
- return false;
- }
- list_->push_back(std::move(in_value));
+ if (std::find(list_->begin(), list_->end(), *in_value) != list_->end())
+ return false;
+
+ list_->push_back(std::move(*in_value));
return true;
}
@@ -1301,15 +1289,12 @@
if (index > list_->size())
return false;
- list_->insert(list_->begin() + index, std::move(in_value));
+ list_->insert(list_->begin() + index, std::move(*in_value));
return true;
}
ListValue::const_iterator ListValue::Find(const Value& value) const {
- return std::find_if(list_->begin(), list_->end(),
- [&value](const std::unique_ptr<Value>& entry) {
- return *entry == value;
- });
+ return std::find(list_->begin(), list_->end(), value);
}
void ListValue::Swap(ListValue* other) {
diff --git a/base/values.h b/base/values.h
index ace8b43..075bc9f 100644
--- a/base/values.h
+++ b/base/values.h
@@ -49,7 +49,7 @@
class BASE_EXPORT Value {
public:
using DictStorage = base::flat_map<std::string, std::unique_ptr<Value>>;
- using ListStorage = std::vector<std::unique_ptr<Value>>;
+ using ListStorage = std::vector<Value>;
enum class Type {
NONE = 0,
@@ -390,9 +390,15 @@
// Returns the number of Values in this list.
size_t GetSize() const { return list_->size(); }
+ // Returns the capacity of storage for Values in this list.
+ size_t capacity() const { return list_->capacity(); }
+
// Returns whether the list is empty.
bool empty() const { return list_->empty(); }
+ // Reserves storage for at least |n| values.
+ void Reserve(size_t n);
+
// 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.
diff --git a/base/values_unittest.cc b/base/values_unittest.cc
index 077f54c..ae91658 100644
--- a/base/values_unittest.cc
+++ b/base/values_unittest.cc
@@ -437,7 +437,7 @@
base::Value not_found_value(false);
ASSERT_NE(mixed_list->end(), mixed_list->Find(sought_value));
- ASSERT_TRUE((*mixed_list->Find(sought_value))->GetAsInteger(&int_value));
+ ASSERT_TRUE((*mixed_list->Find(sought_value)).GetAsInteger(&int_value));
ASSERT_EQ(42, int_value);
ASSERT_EQ(mixed_list->end(), mixed_list->Find(not_found_value));
}
@@ -540,10 +540,10 @@
{
ListValue list;
auto value = MakeUnique<Value>();
- Value* original_value = value.get();
+ Value original_value = *value;
list.Append(std::move(value));
size_t index = 0;
- list.Remove(*original_value, &index);
+ list.Remove(original_value, &index);
EXPECT_EQ(0U, index);
EXPECT_EQ(0U, list.GetSize());
}