Bug 1575479 - Add support for STL iterators and range-based for to nsBaseHashtable. r=froydnj

Differential Revision: https://phabricator.services.mozilla.com/D44982

--HG--
extra : moz-landing-system : lando

Author: sigiesec

xpcom/ds/PLDHashTable.cpp

@@ -729,6 +729,36 @@ PLDHashTable::Iterator::Iterator(PLDHashTable* aTable)
   }
 }
 
+PLDHashTable::Iterator::Iterator(PLDHashTable* aTable, EndIteratorTag aTag)
+    : mTable(aTable),
+      mCurrent(mTable->mEntryStore.SlotForIndex(0, mTable->mEntrySize,
+                                                mTable->Capacity())),
+      mNexts(mTable->EntryCount()),
+      mNextsLimit(mTable->EntryCount()),
+      mHaveRemoved(false),
+      mEntrySize(aTable->mEntrySize) {
+#ifdef DEBUG
+  mTable->mChecker.StartReadOp();
+#endif
+
+  MOZ_ASSERT(Done());
+}
+
+PLDHashTable::Iterator::Iterator(const Iterator& aOther)
+    : mTable(aOther.mTable),
+      mCurrent(aOther.mCurrent),
+      mNexts(aOther.mNexts),
+      mNextsLimit(aOther.mNextsLimit),
+      mHaveRemoved(aOther.mHaveRemoved),
+      mEntrySize(aOther.mEntrySize) {
+  // TODO: Is this necessary?
+  MOZ_ASSERT(!mHaveRemoved);
+
+#ifdef DEBUG
+  mTable->mChecker.StartReadOp();
+#endif
+}
+
 PLDHashTable::Iterator::~Iterator() {
   if (mTable) {
     if (mHaveRemoved) {

xpcom/ds/PLDHashTable.h

@@ -571,6 +571,8 @@ class PLDHashTable {
   class Iterator {
    public:
     explicit Iterator(PLDHashTable* aTable);
+    struct EndIteratorTag {};
+    Iterator(PLDHashTable* aTable, EndIteratorTag aTag);
     Iterator(Iterator&& aOther);
     ~Iterator();
 
@@ -591,6 +593,13 @@ class PLDHashTable {
     // must not be called on that entry afterwards.
     void Remove();
 
+    bool operator==(const Iterator& aOther) const {
+      MOZ_ASSERT(mTable == aOther.mTable);
+      return mNexts == aOther.mNexts;
+    }
+
+    Iterator Clone() const { return {*this}; }
+
    protected:
     PLDHashTable* mTable;  // Main table pointer.
 
@@ -607,7 +616,7 @@ class PLDHashTable {
     void MoveToNextLiveEntry();
 
     Iterator() = delete;
-    Iterator(const Iterator&) = delete;
+    Iterator(const Iterator&);
     Iterator& operator=(const Iterator&) = delete;
     Iterator& operator=(const Iterator&&) = delete;
   };

xpcom/ds/nsBaseHashtable.h

@@ -396,6 +396,91 @@ class nsBaseHashtable
     return Iterator(const_cast<nsBaseHashtable*>(this));
   }
 
+  // STL-style iterators to allow the use in range-based for loops, e.g.
+  template <typename T>
+  class base_iterator
+      : public std::iterator<std::forward_iterator_tag, T, int32_t> {
+   public:
+    using typename std::iterator<std::forward_iterator_tag, T,
+                                 int32_t>::value_type;
+    using typename std::iterator<std::forward_iterator_tag, T,
+                                 int32_t>::difference_type;
+
+    using iterator_type = base_iterator;
+    using const_iterator_type = base_iterator<const T>;
+
+    using EndIteratorTag = PLDHashTable::Iterator::EndIteratorTag;
+
+    base_iterator(base_iterator&& aOther) = default;
+
+    base_iterator& operator=(base_iterator&& aOther) {
+      // User-defined because the move assignment operator is deleted in
+      // PLDHashtable::Iterator.
+      return operator=(static_cast<const base_iterator&>(aOther));
+    }
+
+    base_iterator(const base_iterator& aOther)
+        : mIterator{aOther.mIterator.Clone()} {}
+    base_iterator& operator=(const base_iterator& aOther) {
+      // Since PLDHashTable::Iterator has no assignment operator, we destroy and
+      // recreate mIterator.
+      mIterator.~Iterator();
+      new (&mIterator) PLDHashTable::Iterator(aOther.mIterator.Clone());
+      return *this;
+    }
+
+    explicit base_iterator(PLDHashTable::Iterator aFrom)
+        : mIterator{std::move(aFrom)} {}
+
+    explicit base_iterator(const nsBaseHashtable* aTable)
+        : mIterator{&const_cast<nsBaseHashtable*>(aTable)->mTable} {}
+
+    base_iterator(const nsBaseHashtable* aTable, EndIteratorTag aTag)
+        : mIterator{&const_cast<nsBaseHashtable*>(aTable)->mTable, aTag} {}
+
+    bool operator==(const iterator_type& aRhs) const {
+      return mIterator == aRhs.mIterator;
+    }
+    bool operator!=(const iterator_type& aRhs) const {
+      return !(*this == aRhs);
+    }
+
+    value_type* operator->() const {
+      return static_cast<value_type*>(mIterator.Get());
+    }
+    value_type& operator*() const {
+      return *static_cast<value_type*>(mIterator.Get());
+    }
+
+    iterator_type& operator++() {
+      mIterator.Next();
+      return *this;
+    }
+    iterator_type operator++(int) {
+      iterator_type it = *this;
+      ++*this;
+      return it;
+    }
+
+    operator const_iterator_type() const {
+      return const_iterator_type{mIterator.Clone()};
+    }
+
+   private:
+    PLDHashTable::Iterator mIterator;
+  };
+  using const_iterator = base_iterator<const EntryType>;
+  using iterator = base_iterator<EntryType>;
+
+  iterator begin() { return iterator{this}; }
+  const_iterator begin() const { return const_iterator{this}; }
+  const_iterator cbegin() const { return begin(); }
+  iterator end() { return iterator{this, typename iterator::EndIteratorTag{}}; }
+  const_iterator end() const {
+    return const_iterator{this, typename const_iterator::EndIteratorTag{}};
+  }
+  const_iterator cend() const { return end(); }
+
   /**
    * reset the hashtable, removing all entries
    */

xpcom/tests/gtest/TestHashtables.cpp

@@ -17,6 +17,8 @@
 
 #include "gtest/gtest.h"
 
+#include <numeric>
+
 namespace TestHashtables {
 
 class TestUniChar  // for nsClassHashtable
@@ -35,6 +37,11 @@ class TestUniChar  // for nsClassHashtable
 struct EntityNode {
   const char* mStr;  // never owns buffer
   uint32_t mUnicode;
+
+  bool operator<(const EntityNode& aOther) const {
+    return mUnicode < aOther.mUnicode ||
+           (mUnicode == aOther.mUnicode && strcmp(mStr, aOther.mStr) < 0);
+  }
 };
 
 static const EntityNode gEntities[] = {
@@ -286,6 +293,68 @@ TEST(Hashtables, DataHashtable)
   ASSERT_EQ(count, uint32_t(0));
 }
 
+TEST(Hashtables, DataHashtable_STLIterators)
+{
+  nsDataHashtable<nsUint32HashKey, const char*> UniToEntity(ENTITY_COUNT);
+
+  for (auto& entity : gEntities) {
+    UniToEntity.Put(entity.mUnicode, entity.mStr);
+  }
+
+  // operators, including conversion from iterator to const_iterator
+  nsDataHashtable<nsUint32HashKey, const char*>::const_iterator ci =
+      UniToEntity.begin();
+  ++ci;
+  ASSERT_EQ(1, std::distance(UniToEntity.cbegin(), ci++));
+  ASSERT_EQ(2, std::distance(UniToEntity.cbegin(), ci));
+  ASSERT_TRUE(ci == ci);
+  auto otherCi = ci;
+  ++otherCi;
+  ++ci;
+  ASSERT_TRUE(&*ci == &*otherCi);
+
+  // STL algorithms (just to check that the iterator sufficiently conforms with
+  // the actual syntactical requirements of those algorithms).
+  std::for_each(UniToEntity.cbegin(), UniToEntity.cend(),
+                [](const auto& entry) {});
+  std::find_if(UniToEntity.cbegin(), UniToEntity.cend(),
+               [](const auto& entry) { return entry.GetKey() == 42; });
+  std::accumulate(
+      UniToEntity.cbegin(), UniToEntity.cend(), 0u,
+      [](size_t sum, const auto& entry) { return sum + entry.GetKey(); });
+  std::any_of(UniToEntity.cbegin(), UniToEntity.cend(),
+              [](const auto& entry) { return entry.GetKey() == 42; });
+  std::max_element(UniToEntity.cbegin(), UniToEntity.cend(),
+                   [](const auto& lhs, const auto& rhs) {
+                     return lhs.GetKey() > rhs.GetKey();
+                   });
+
+  // const range-based for
+  {
+    std::set<EntityNode> entities(gEntities, gEntities + ENTITY_COUNT);
+    for (const auto& entity :
+         const_cast<const nsDataHashtable<nsUint32HashKey, const char*>&>(
+             UniToEntity)) {
+      ASSERT_EQ(1u,
+                entities.erase(EntityNode{entity.GetData(), entity.GetKey()}));
+    }
+    ASSERT_TRUE(entities.empty());
+  }
+
+  // non-const range-based for
+  {
+    std::set<EntityNode> entities(gEntities, gEntities + ENTITY_COUNT);
+    for (auto& entity : UniToEntity) {
+      ASSERT_EQ(1u,
+                entities.erase(EntityNode{entity.GetData(), entity.GetKey()}));
+
+      entity.SetData(nullptr);
+      ASSERT_EQ(nullptr, entity.GetData());
+    }
+    ASSERT_TRUE(entities.empty());
+  }
+}
+
 TEST(Hashtables, ClassHashtable)
 {
   // check a class-hashtable
@@ -319,6 +388,46 @@ TEST(Hashtables, ClassHashtable)
   ASSERT_EQ(count, uint32_t(0));
 }
 
+TEST(Hashtables, ClassHashtable_RangeBasedFor)
+{
+  // check a class-hashtable
+  nsClassHashtable<nsCStringHashKey, TestUniChar> EntToUniClass(ENTITY_COUNT);
+
+  for (auto& entity : gEntities) {
+    auto* temp = new TestUniChar(entity.mUnicode);
+    EntToUniClass.Put(nsDependentCString(entity.mStr), temp);
+  }
+
+  // const range-based for
+  {
+    std::set<EntityNode> entities(gEntities, gEntities + ENTITY_COUNT);
+    for (const auto& entity :
+         const_cast<const nsClassHashtable<nsCStringHashKey, TestUniChar>&>(
+             EntToUniClass)) {
+      const char* str;
+      entity.GetKey().GetData(&str);
+      ASSERT_EQ(1u,
+                entities.erase(EntityNode{str, entity.GetData()->GetChar()}));
+    }
+    ASSERT_TRUE(entities.empty());
+  }
+
+  // non-const range-based for
+  {
+    std::set<EntityNode> entities(gEntities, gEntities + ENTITY_COUNT);
+    for (auto& entity : EntToUniClass) {
+      const char* str;
+      entity.GetKey().GetData(&str);
+      ASSERT_EQ(1u,
+                entities.erase(EntityNode{str, entity.GetData()->GetChar()}));
+
+      entity.SetData(nsAutoPtr<TestUniChar>{});
+      ASSERT_EQ(nullptr, entity.GetData());
+    }
+    ASSERT_TRUE(entities.empty());
+  }
+}
+
 TEST(Hashtables, DataHashtableWithInterfaceKey)
 {
   // check a data-hashtable with an interface key