From 46992f0262726385fff32da88073b2836caa780c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Wilken=20D=C3=B6rrie?= Date: Thu, 20 Feb 2020 11:25:47 +0000 Subject: [PATCH] [base] Use Checked Iterators in ListValue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While base::Value switched to checked iterators in its list API, base::ListValue still used unchecked std::vector::iterators. This change fixes this oversight and slightly improves the checked iterator interface by making the comparison operators non-members, thus allowing implicit conversions on both arguments. Bug: 990059 Change-Id: I7b3bb4573300fe7f391c6b911817448f8aee7519 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2060672 Commit-Queue: Jan Wilken Dörrie Reviewed-by: Chris Palmer Reviewed-by: Daniel Cheng Cr-Commit-Position: refs/heads/master@{#743050} --- base/BUILD.gn | 1 + base/containers/checked_iterators.h | 46 ++++++----- base/containers/checked_iterators_unittest.cc | 80 +++++++++++++++++++ base/values.cc | 9 ++- base/values.h | 12 +-- 5 files changed, 118 insertions(+), 30 deletions(-) create mode 100644 base/containers/checked_iterators_unittest.cc diff --git a/base/BUILD.gn b/base/BUILD.gn index e9e606ef1e515b..41e443df42730f 100644 --- a/base/BUILD.gn +++ b/base/BUILD.gn @@ -2487,6 +2487,7 @@ test("base_unittests") { "component_export_unittest.cc", "containers/adapters_unittest.cc", "containers/buffer_iterator_unittest.cc", + "containers/checked_iterators_unittest.cc", "containers/checked_range_unittest.cc", "containers/circular_deque_unittest.cc", "containers/flat_map_unittest.cc", diff --git a/base/containers/checked_iterators.h b/base/containers/checked_iterators.h index cdfd2909d4feb6..66d927d24654f4 100644 --- a/base/containers/checked_iterators.h +++ b/base/containers/checked_iterators.h @@ -60,34 +60,39 @@ class CheckedContiguousIterator { constexpr CheckedContiguousIterator& operator=( const CheckedContiguousIterator& other) = default; - constexpr bool operator==(const CheckedContiguousIterator& other) const { - CheckComparable(other); - return current_ == other.current_; + friend constexpr bool operator==(const CheckedContiguousIterator& lhs, + const CheckedContiguousIterator& rhs) { + lhs.CheckComparable(rhs); + return lhs.current_ == rhs.current_; } - constexpr bool operator!=(const CheckedContiguousIterator& other) const { - CheckComparable(other); - return current_ != other.current_; + friend constexpr bool operator!=(const CheckedContiguousIterator& lhs, + const CheckedContiguousIterator& rhs) { + lhs.CheckComparable(rhs); + return lhs.current_ != rhs.current_; } - constexpr bool operator<(const CheckedContiguousIterator& other) const { - CheckComparable(other); - return current_ < other.current_; + friend constexpr bool operator<(const CheckedContiguousIterator& lhs, + const CheckedContiguousIterator& rhs) { + lhs.CheckComparable(rhs); + return lhs.current_ < rhs.current_; } - constexpr bool operator<=(const CheckedContiguousIterator& other) const { - CheckComparable(other); - return current_ <= other.current_; + friend constexpr bool operator<=(const CheckedContiguousIterator& lhs, + const CheckedContiguousIterator& rhs) { + lhs.CheckComparable(rhs); + return lhs.current_ <= rhs.current_; } - - constexpr bool operator>(const CheckedContiguousIterator& other) const { - CheckComparable(other); - return current_ > other.current_; + friend constexpr bool operator>(const CheckedContiguousIterator& lhs, + const CheckedContiguousIterator& rhs) { + lhs.CheckComparable(rhs); + return lhs.current_ > rhs.current_; } - constexpr bool operator>=(const CheckedContiguousIterator& other) const { - CheckComparable(other); - return current_ >= other.current_; + friend constexpr bool operator>=(const CheckedContiguousIterator& lhs, + const CheckedContiguousIterator& rhs) { + lhs.CheckComparable(rhs); + return lhs.current_ >= rhs.current_; } constexpr CheckedContiguousIterator& operator++() { @@ -149,8 +154,7 @@ class CheckedContiguousIterator { constexpr friend difference_type operator-( const CheckedContiguousIterator& lhs, const CheckedContiguousIterator& rhs) { - CHECK_EQ(lhs.start_, rhs.start_); - CHECK_EQ(lhs.end_, rhs.end_); + lhs.CheckComparable(rhs); return lhs.current_ - rhs.current_; } diff --git a/base/containers/checked_iterators_unittest.cc b/base/containers/checked_iterators_unittest.cc new file mode 100644 index 00000000000000..9004a020b52cd3 --- /dev/null +++ b/base/containers/checked_iterators_unittest.cc @@ -0,0 +1,80 @@ +// Copyright 2020 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/containers/checked_iterators.h" + +#include "testing/gtest/include/gtest/gtest.h" + +namespace base { + +// Checks that constexpr CheckedContiguousConstIterators can be compared at +// compile time. +TEST(CheckedContiguousIterator, StaticComparisonOperators) { + static constexpr int arr[] = {0}; + + constexpr CheckedContiguousConstIterator begin(arr, arr, arr + 1); + constexpr CheckedContiguousConstIterator end(arr, arr + 1, arr + 1); + + static_assert(begin == begin, ""); + static_assert(end == end, ""); + + static_assert(begin != end, ""); + static_assert(end != begin, ""); + + static_assert(begin < end, ""); + + static_assert(begin <= begin, ""); + static_assert(begin <= end, ""); + static_assert(end <= end, ""); + + static_assert(end > begin, ""); + + static_assert(end >= end, ""); + static_assert(end >= begin, ""); + static_assert(begin >= begin, ""); +} + +// Checks that comparison between iterators and const iterators works in both +// directions. +TEST(CheckedContiguousIterator, ConvertingComparisonOperators) { + static int arr[] = {0}; + + CheckedContiguousIterator begin(arr, arr, arr + 1); + CheckedContiguousConstIterator cbegin(arr, arr, arr + 1); + + CheckedContiguousIterator end(arr, arr + 1, arr + 1); + CheckedContiguousConstIterator cend(arr, arr + 1, arr + 1); + + EXPECT_EQ(begin, cbegin); + EXPECT_EQ(cbegin, begin); + EXPECT_EQ(end, cend); + EXPECT_EQ(cend, end); + + EXPECT_NE(begin, cend); + EXPECT_NE(cbegin, end); + EXPECT_NE(end, cbegin); + EXPECT_NE(cend, begin); + + EXPECT_LT(begin, cend); + EXPECT_LT(cbegin, end); + + EXPECT_LE(begin, cbegin); + EXPECT_LE(cbegin, begin); + EXPECT_LE(begin, cend); + EXPECT_LE(cbegin, end); + EXPECT_LE(end, cend); + EXPECT_LE(cend, end); + + EXPECT_GT(end, cbegin); + EXPECT_GT(cend, begin); + + EXPECT_GE(end, cend); + EXPECT_GE(cend, end); + EXPECT_GE(end, cbegin); + EXPECT_GE(cend, begin); + EXPECT_GE(begin, cbegin); + EXPECT_GE(cbegin, begin); +} + +} // namespace base diff --git a/base/values.cc b/base/values.cc index fbc859c6518d30..ecca445840a866 100644 --- a/base/values.cc +++ b/base/values.cc @@ -414,7 +414,7 @@ CheckedContiguousIterator Value::Insert( bool Value::EraseListIter(CheckedContiguousConstIterator iter) { CHECK(is_list()); - const auto offset = iter - make_span(list_).begin(); + const auto offset = iter - ListView(list_).begin(); auto list_iter = list_.begin() + offset; if (list_iter == list_.end()) return false; @@ -1752,7 +1752,10 @@ ListValue::iterator ListValue::Erase(iterator iter, if (out_value) *out_value = std::make_unique(std::move(*iter)); - return list_.erase(iter); + auto list_iter = list_.begin() + (iter - GetList().begin()); + CHECK(list_iter != list_.end()); + list_iter = list_.erase(list_iter); + return GetList().begin() + (list_iter - list_.begin()); } void ListValue::Append(std::unique_ptr in_value) { @@ -1810,7 +1813,7 @@ bool ListValue::Insert(size_t index, std::unique_ptr in_value) { } ListValue::const_iterator ListValue::Find(const Value& value) const { - return std::find(list_.begin(), list_.end(), value); + return std::find(GetList().begin(), GetList().end(), value); } void ListValue::Swap(ListValue* other) { diff --git a/base/values.h b/base/values.h index a04bffb670b43c..82d05a3a384b78 100644 --- a/base/values.h +++ b/base/values.h @@ -784,8 +784,8 @@ class BASE_EXPORT DictionaryValue : public Value { // This type of Value represents a list of other Value values. class BASE_EXPORT ListValue : public Value { public: - using const_iterator = ListStorage::const_iterator; - using iterator = ListStorage::iterator; + using const_iterator = ListView::const_iterator; + using iterator = ListView::iterator; // Returns |value| if it is a list, nullptr otherwise. static std::unique_ptr From(std::unique_ptr value); @@ -910,14 +910,14 @@ class BASE_EXPORT ListValue : public Value { // Iteration. // DEPRECATED, use GetList()::begin() instead. - iterator begin() { return list_.begin(); } + iterator begin() { return GetList().begin(); } // DEPRECATED, use GetList()::end() instead. - iterator end() { return list_.end(); } + iterator end() { return GetList().end(); } // DEPRECATED, use GetList()::begin() instead. - const_iterator begin() const { return list_.begin(); } + const_iterator begin() const { return GetList().begin(); } // DEPRECATED, use GetList()::end() instead. - const_iterator end() const { return list_.end(); } + const_iterator end() const { return GetList().end(); } // DEPRECATED, use Value::Clone() instead. // TODO(crbug.com/646113): Delete this and migrate callsites.