Make AnnotationList's iterator compliant to input iterator

This CL make the iterators implemented by AnnotationList compliant to
the requirements imposed by the C++ standard on input iterators.

Change-Id: I263c94a97f5bcd7edd5ef4d8b65fa28b11876974
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5093147
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>

client/annotation_list.cc

@@ -19,6 +19,46 @@
 
 namespace crashpad {
 
+template <typename T>
+T* AnnotationList::IteratorBase<T>::operator*() const {
+  CHECK_NE(curr_, tail_);
+  return curr_;
+}
+
+template <typename T>
+T* AnnotationList::IteratorBase<T>::operator->() const {
+  CHECK_NE(curr_, tail_);
+  return curr_;
+}
+
+template <typename T>
+AnnotationList::IteratorBase<T>& AnnotationList::IteratorBase<T>::operator++() {
+  CHECK_NE(curr_, tail_);
+  curr_ = curr_->GetLinkNode();
+  return *this;
+}
+
+template <typename T>
+AnnotationList::IteratorBase<T> AnnotationList::IteratorBase<T>::operator++(
+    int) {
+  T* const old_curr = curr_;
+  ++(*this);
+  return IteratorBase(old_curr, tail_);
+}
+
+template <typename T>
+bool AnnotationList::IteratorBase<T>::operator!=(
+    const IteratorBase& other) const {
+  return !(*this == other);
+}
+
+template <typename T>
+AnnotationList::IteratorBase<T>::IteratorBase(T* head, const Annotation* tail)
+    : curr_(head), tail_(tail) {}
+
+template class AnnotationList::IteratorBase<Annotation>;
+template class AnnotationList::IteratorBase<const Annotation>;
+
 AnnotationList::AnnotationList()
     : tail_pointer_(&tail_),
       head_(Annotation::Type::kInvalid, nullptr, nullptr),
@@ -65,49 +105,6 @@ void AnnotationList::Add(Annotation* annotation) {
   }
 }
 
-AnnotationList::Iterator::Iterator(Annotation* head, const Annotation* tail)
-    : curr_(head), tail_(tail) {}
-
-AnnotationList::Iterator::~Iterator() = default;
-
-Annotation* AnnotationList::Iterator::operator*() const {
-  CHECK_NE(curr_, tail_);
-  return curr_;
-}
-
-AnnotationList::Iterator& AnnotationList::Iterator::operator++() {
-  CHECK_NE(curr_, tail_);
-  curr_ = curr_->GetLinkNode();
-  return *this;
-}
-
-bool AnnotationList::Iterator::operator==(
-    const AnnotationList::Iterator& other) const {
-  return curr_ == other.curr_;
-}
-
-AnnotationList::ConstIterator::ConstIterator(const Annotation* head,
-                                             const Annotation* tail)
-    : curr_(head), tail_(tail) {}
-
-AnnotationList::ConstIterator::~ConstIterator() = default;
-
-const Annotation* AnnotationList::ConstIterator::operator*() const {
-  CHECK_NE(curr_, tail_);
-  return curr_;
-}
-
-AnnotationList::ConstIterator& AnnotationList::ConstIterator::operator++() {
-  CHECK_NE(curr_, tail_);
-  curr_ = curr_->GetLinkNode();
-  return *this;
-}
-
-bool AnnotationList::ConstIterator::operator==(
-    const AnnotationList::ConstIterator& other) const {
-  return curr_ == other.curr_;
-}
-
 AnnotationList::Iterator AnnotationList::begin() {
   return Iterator(head_.GetLinkNode(), tail_pointer_);
 }

client/annotation_list.h

@@ -15,6 +15,8 @@
 #ifndef CRASHPAD_CLIENT_ANNOTATION_LIST_H_
 #define CRASHPAD_CLIENT_ANNOTATION_LIST_H_
 
+#include <iterator>
+
 #include "build/build_config.h"
 #include "client/annotation.h"
 
@@ -61,47 +63,46 @@ class AnnotationList {
   void Add(Annotation* annotation);
 
   //! \brief An InputIterator for the AnnotationList.
-  class Iterator {
+  template <typename T>
+  class IteratorBase {
    public:
-    ~Iterator();
+    using difference_type = signed int;
+    using value_type = T*;
+    using reference = T*;
+    using pointer = void;
+    using iterator_category = std::input_iterator_tag;
 
-    Annotation* operator*() const;
-    Iterator& operator++();
-    bool operator==(const Iterator& other) const;
-    bool operator!=(const Iterator& other) const { return !(*this == other); }
+    IteratorBase(const IteratorBase& other) = default;
+    IteratorBase(IteratorBase&& other) = default;
 
-   private:
-    friend class AnnotationList;
-    Iterator(Annotation* head, const Annotation* tail);
+    ~IteratorBase() = default;
 
-    Annotation* curr_;
-    const Annotation* const tail_;
+    IteratorBase& operator=(const IteratorBase& other) = default;
+    IteratorBase& operator=(IteratorBase&& other) = default;
 
-    // Copy and assign are required.
-  };
+    T* operator*() const;
+    T* operator->() const;
 
-  //! \brief An InputIterator for iterating a const AnnotationList.
-  class ConstIterator {
-   public:
-    ~ConstIterator();
+    IteratorBase& operator++();
+    IteratorBase operator++(int);
 
-    const Annotation* operator*() const;
-    ConstIterator& operator++();
-    bool operator==(const ConstIterator& other) const;
-    bool operator!=(const ConstIterator& other) const {
-      return !(*this == other);
+    bool operator==(const IteratorBase& other) const {
+      return curr_ == other.curr_;
     }
 
+    bool operator!=(const IteratorBase& other) const;
+
    private:
     friend class AnnotationList;
-    ConstIterator(const Annotation* head, const Annotation* tail);
+    IteratorBase(T* head, const Annotation* tail);
 
-    const Annotation* curr_;
-    const Annotation* const tail_;
-
-    // Copy and assign are required.
+    T* curr_ = nullptr;
+    const Annotation* tail_ = nullptr;
   };
 
+  using Iterator = IteratorBase<Annotation>;
+  using ConstIterator = IteratorBase<const Annotation>;
+
   //! \brief Returns an iterator to the first element of the annotation list.
   Iterator begin();
   ConstIterator begin() const { return cbegin(); }

client/annotation_list_test.cc

@@ -14,7 +14,10 @@
 
 #include "client/annotation.h"
 
+#include <algorithm>
+#include <iterator>
 #include <string>
+#include <type_traits>
 #include <vector>
 
 #include "base/rand_util.h"
@@ -27,6 +30,52 @@ namespace crashpad {
 namespace test {
 namespace {
 
+#if (__cplusplus >= 202002L)
+template <typename Iterator>
+  requires std::input_iterator<Iterator>
+void VerifyIsInputIterator(Iterator) {}
+#else
+template <typename Iterator>
+struct IsLegacyIteratorImpl {
+  static constexpr bool value =
+      std::is_copy_constructible_v<Iterator> &&
+      std::is_copy_assignable_v<Iterator> && std::is_destructible_v<Iterator> &&
+      std::is_swappable_v<Iterator> &&
+      // check that std::iterator_traits has the necessary types (check only one
+      // needed as std::iterator is required to define only if all are defined)
+      !std::is_same_v<typename std::iterator_traits<Iterator>::reference,
+                      void> &&
+      std::is_same_v<decltype(++std::declval<Iterator>()), Iterator&> &&
+      !std::is_same_v<decltype(*std::declval<Iterator>()), void>;
+};
+
+template <typename Iterator>
+struct IsLegacyInputIteratorImpl {
+  static constexpr bool value =
+      IsLegacyIteratorImpl<Iterator>::value &&
+      std::is_base_of_v<
+          std::input_iterator_tag,
+          typename std::iterator_traits<Iterator>::iterator_category> &&
+      std::is_convertible_v<decltype(std::declval<Iterator>() !=
+                                     std::declval<Iterator>()),
+                            bool> &&
+      std::is_convertible_v<decltype(std::declval<Iterator>() ==
+                                     std::declval<Iterator>()),
+                            bool> &&
+      std::is_same_v<decltype(*std::declval<Iterator>()),
+                     typename std::iterator_traits<Iterator>::reference> &&
+      std::is_same_v<decltype(++std::declval<Iterator>()), Iterator&> &&
+      std::is_same_v<decltype(std::declval<Iterator>()++), Iterator> &&
+      std::is_same_v<decltype(*(++std::declval<Iterator>())),
+                     typename std::iterator_traits<Iterator>::reference>;
+};
+
+template <typename Iterator>
+void VerifyIsInputIterator(Iterator) {
+  static_assert(IsLegacyInputIteratorImpl<Iterator>::value);
+}
+#endif
+
 TEST(AnnotationListStatic, Register) {
   ASSERT_FALSE(AnnotationList::Get());
   EXPECT_TRUE(AnnotationList::Register());
@@ -222,6 +271,23 @@ TEST_F(AnnotationList, IteratorMultipleAnnotationsInsertedAndRemoved) {
   EXPECT_EQ(const_iterator, annotations_.cend());
 }
 
+TEST_F(AnnotationList, IteratorIsInputIterator) {
+  one_.Set("1");
+  two_.Set("2");
+
+  // Check explicitly that the iterators meet the interface of an input
+  // iterator.
+  VerifyIsInputIterator(annotations_.begin());
+  VerifyIsInputIterator(annotations_.cbegin());
+  VerifyIsInputIterator(annotations_.end());
+  VerifyIsInputIterator(annotations_.cend());
+
+  // Additionally verify that std::distance accepts the iterators. It requires
+  // the iterators to comply to the input iterator interface.
+  EXPECT_EQ(std::distance(annotations_.begin(), annotations_.end()), 2);
+  EXPECT_EQ(std::distance(annotations_.cbegin(), annotations_.cend()), 2);
+}
+
 class RaceThread : public Thread {
  public:
   explicit RaceThread(test::AnnotationList* test) : Thread(), test_(test) {}