[base] Use Checked Iterators in ListValue

While base::Value switched to checked iterators in its list API,
base::ListValue still used unchecked std::vector<Value>::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 <jdoerrie@chromium.org>
Reviewed-by: Chris Palmer <palmer@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#743050}

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",

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_;
   }
 

base/containers/checked_iterators_unittest.cc

+// 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<int> begin(arr, arr, arr + 1);
+  constexpr CheckedContiguousConstIterator<int> 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<int> begin(arr, arr, arr + 1);
+  CheckedContiguousConstIterator<int> cbegin(arr, arr, arr + 1);
+
+  CheckedContiguousIterator<int> end(arr, arr + 1, arr + 1);
+  CheckedContiguousConstIterator<int> 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

base/values.cc

@@ -414,7 +414,7 @@ CheckedContiguousIterator<Value> Value::Insert(
 
 bool Value::EraseListIter(CheckedContiguousConstIterator<Value> 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<Value>(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<Value> in_value) {
@@ -1810,7 +1813,7 @@ bool ListValue::Insert(size_t index, std::unique_ptr<Value> 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) {

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<ListValue> From(std::unique_ptr<Value> 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.