From 1b5be2b03f93aadc12b4308d91a5d8a70321ebfd Mon Sep 17 00:00:00 2001 From: Simon Giesecke Date: Mon, 9 Sep 2019 14:27:24 +0000 Subject: [PATCH] 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 --- xpcom/ds/PLDHashTable.cpp | 30 ++++++++ xpcom/ds/PLDHashTable.h | 11 ++- xpcom/ds/nsBaseHashtable.h | 85 +++++++++++++++++++++ xpcom/tests/gtest/TestHashtables.cpp | 109 +++++++++++++++++++++++++++ 4 files changed, 234 insertions(+), 1 deletion(-) diff --git a/xpcom/ds/PLDHashTable.cpp b/xpcom/ds/PLDHashTable.cpp index 002699a9fe48..40c845aff1cd 100644 --- a/xpcom/ds/PLDHashTable.cpp +++ b/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) { diff --git a/xpcom/ds/PLDHashTable.h b/xpcom/ds/PLDHashTable.h index 8346aa113f80..1a65ae694f4e 100644 --- a/xpcom/ds/PLDHashTable.h +++ b/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; }; diff --git a/xpcom/ds/nsBaseHashtable.h b/xpcom/ds/nsBaseHashtable.h index fe935bf2dd5a..90af9d855342 100644 --- a/xpcom/ds/nsBaseHashtable.h +++ b/xpcom/ds/nsBaseHashtable.h @@ -396,6 +396,91 @@ class nsBaseHashtable return Iterator(const_cast(this)); } + // STL-style iterators to allow the use in range-based for loops, e.g. + template + class base_iterator + : public std::iterator { + public: + using typename std::iterator::value_type; + using typename std::iterator::difference_type; + + using iterator_type = base_iterator; + using const_iterator_type = base_iterator; + + 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(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(aTable)->mTable} {} + + base_iterator(const nsBaseHashtable* aTable, EndIteratorTag aTag) + : mIterator{&const_cast(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(mIterator.Get()); + } + value_type& operator*() const { + return *static_cast(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; + using iterator = base_iterator; + + 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 */ diff --git a/xpcom/tests/gtest/TestHashtables.cpp b/xpcom/tests/gtest/TestHashtables.cpp index 26779f0fbd4d..dfb94118ba49 100644 --- a/xpcom/tests/gtest/TestHashtables.cpp +++ b/xpcom/tests/gtest/TestHashtables.cpp @@ -17,6 +17,8 @@ #include "gtest/gtest.h" +#include + 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 UniToEntity(ENTITY_COUNT); + + for (auto& entity : gEntities) { + UniToEntity.Put(entity.mUnicode, entity.mStr); + } + + // operators, including conversion from iterator to const_iterator + nsDataHashtable::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 entities(gEntities, gEntities + ENTITY_COUNT); + for (const auto& entity : + const_cast&>( + UniToEntity)) { + ASSERT_EQ(1u, + entities.erase(EntityNode{entity.GetData(), entity.GetKey()})); + } + ASSERT_TRUE(entities.empty()); + } + + // non-const range-based for + { + std::set 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 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 entities(gEntities, gEntities + ENTITY_COUNT); + for (const auto& entity : + const_cast&>( + 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 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{}); + ASSERT_EQ(nullptr, entity.GetData()); + } + ASSERT_TRUE(entities.empty()); + } +} + TEST(Hashtables, DataHashtableWithInterfaceKey) { // check a data-hashtable with an interface key