From 3012a7db3494d3623c3158500d26987a0f2e99bf Mon Sep 17 00:00:00 2001 From: Simon Giesecke Date: Tue, 9 Feb 2021 18:19:35 +0000 Subject: [PATCH] Bug 1688833 - Make nsBaseHashtable::WithEntryHandle public and complete its design. r=xpcom-reviewers,nika Implements the missing handle functions (OrInsertWith, OrUpdateWith), and harmonizes functions to return a reference to the data. Adds unit tests. Differential Revision: https://phabricator.services.mozilla.com/D99764 --- xpcom/ds/nsBaseHashtable.h | 136 +++++++++++++++++++++++---- xpcom/ds/nsTHashtable.h | 4 +- xpcom/tests/gtest/TestHashtables.cpp | 86 ++++++++++++++++- 3 files changed, 203 insertions(+), 23 deletions(-) diff --git a/xpcom/ds/nsBaseHashtable.h b/xpcom/ds/nsBaseHashtable.h index db2a07d4dded..8b3b0f61e11a 100644 --- a/xpcom/ds/nsBaseHashtable.h +++ b/xpcom/ds/nsBaseHashtable.h @@ -195,7 +195,7 @@ class nsBaseHashtable * This function can only be used if DataType is default-constructible. Use * LookupForAdd with non-default-constructible DataType for now. * - * TODO: Add a function GetOrInsertFrom that will use a function for + * TODO: Add a function GetOrInsertWith that will use a function for * DataType construction. */ DataType& GetOrInsert(const KeyType& aKey) { @@ -477,12 +477,24 @@ class nsBaseHashtable return EntryPtr(*this, ent, count == Count()); } - protected: - /* - * The new API for LookupForAdd (WithEntryHandle). - * - * TODO: Merge with the old API, bug 1688833. + /** + * Used by WithEntryHandle as the argument type to its functor. It is + * associated with the Key passed to WithEntryHandle and manages only the + * potential entry with that key. Note that in case no modifying operations + * are called on the handle, the state of the hashtable remains unchanged, + * i.e. WithEntryHandle does not modify the hashtable itself. * + * Provides query functions (Key, HasEntry/operator bool, Data) and + * modifying operations for inserting new entries (Insert), updating existing + * entries (Update) and removing existing entries (Remove). They have + * debug-only assertion that fail when the state of the entry doesn't match + * the expectation. There are variants prefixed with "Or" (OrInsert, OrUpdate, + * OrRemove) that are a no-op in case the entry does already exist resp. does + * not exist. There are also variants OrInsertWith and OrUpdateWith that don't + * accept a value, but a functor, which is only called if the operation takes + * place, which should be used if the provision of the value is not trivial + * (e.g. allocates a heap object). Finally, there's InsertOrUpdate that + * handles both existing and non-existing entries. */ class EntryHandle : protected nsTHashtable::EntryHandle { public: @@ -503,28 +515,67 @@ class nsBaseHashtable using Base::Entry; + /** + * Inserts a new entry with the handle's key and the value passed to this + * function. + * + * \pre !HasEntry() + * \post HasEntry() + */ template - void Insert(U&& aData) { + DataType& Insert(U&& aData) { Base::InsertInternal(Converter::Wrap(std::forward(aData))); + return Data(); } + /** + * If it doesn't yet exist, inserts a new entry with the handle's key and + * the value passed to this function. The value is not consumed if no insert + * takes place. + * + * \post HasEntry() + */ template DataType& OrInsert(U&& aData) { if (!HasEntry()) { - Insert(std::forward(aData)); + return Insert(std::forward(aData)); } return Data(); } - // TODO: Add functions InsertFrom and OrInsertFrom which will use a function - // for DataType construction. - - template - void Update(U&& aData) { - MOZ_ASSERT(HasEntry()); - Data() = Converter::Wrap(std::forward(aData)); + /** + * If it doesn't yet exist, inserts a new entry with the handle's key and + * the result of the functor passed to this function. The functor is not + * called if no insert takes place. + * + * \post HasEntry() + */ + template + DataType& OrInsertWith(F&& aFunc) { + if (!HasEntry()) { + return Insert(std::forward(aFunc)()); + } + return Data(); } + /** + * Updates the entry with the handle's key by the value passed to this + * function. + * + * \pre HasEntry() + */ + template + DataType& Update(U&& aData) { + MOZ_RELEASE_ASSERT(HasEntry()); + Data() = Converter::Wrap(std::forward(aData)); + return Data(); + } + + /** + * If an entry with the handle's key already exists, updates its value by + * the value passed to this function. The value is not consumed if no update + * takes place. + */ template void OrUpdate(U&& aData) { if (HasEntry()) { @@ -532,6 +583,25 @@ class nsBaseHashtable } } + /** + * If an entry with the handle's key already exists, updates its value by + * the the result of the functor passed to this function. The functor is not + * called if no update takes place. + */ + template + void OrUpdateWith(F&& aFunc) { + if (HasEntry()) { + Update(std::forward(aFunc)()); + } + } + + /** + * If it does not yet, inserts a new entry with the handle's key and the + * value passed to this function. Otherwise, it updates the entry by the + * value passed to this function. + * + * \post HasEntry() + */ template DataType& InsertOrUpdate(U&& aData) { if (!HasEntry()) { @@ -546,6 +616,11 @@ class nsBaseHashtable using Base::OrRemove; + /** + * Returns a reference to the value of the entry. + * + * \pre HasEntry() + */ DataType& Data() { return Entry()->mData; } private: @@ -554,14 +629,39 @@ class nsBaseHashtable explicit EntryHandle(Base&& aBase) : Base(std::move(aBase)) {} }; + /** + * Performs a scoped operation on the entry for aKey, which may or may not + * exist when the function is called. It calls aFunc with an EntryHandle. The + * result of aFunc is returned as the result of this function. Its return type + * may be void. See the documentation of EntryHandle for the query and + * modifying operations it offers. + * + * A simple use of this function is, e.g., + * + * hashtable.WithEntryHandle(key, [](auto&& entry) { entry.OrInsert(42); }); + * + * \attention It is not safe to perform modifying operations on the hashtable + * other than through the EntryHandle within aFunc, and trying to do so will + * trigger debug assertions, and result in undefined behaviour otherwise. + */ template auto WithEntryHandle(KeyType aKey, F&& aFunc) -> std::invoke_result_t { - return Base::WithEntryHandle(aKey, [&aFunc](auto entryHandle) { - return std::forward(aFunc)(EntryHandle{std::move(entryHandle)}); - }); + return Base::WithEntryHandle( + aKey, [&aFunc](auto entryHandle) -> decltype(auto) { + return std::forward(aFunc)(EntryHandle{std::move(entryHandle)}); + }); } + /** + * Fallible variant of WithEntryHandle, with the following differences: + * - The functor aFunc must accept a Maybe (instead of an + * EntryHandle). + * - In case allocation of the slot for the entry fails, Nothing is passed to + * the functor. + * + * For more details, see the explanation on the non-fallible overload above. + */ template auto WithEntryHandle(KeyType aKey, const fallible_t& aFallible, F&& aFunc) -> std::invoke_result_t&&> { diff --git a/xpcom/ds/nsTHashtable.h b/xpcom/ds/nsTHashtable.h index 06291cd541b1..a88bc6f4e224 100644 --- a/xpcom/ds/nsTHashtable.h +++ b/xpcom/ds/nsTHashtable.h @@ -334,6 +334,7 @@ class MOZ_NEEDS_NO_VTABLE_TYPE nsTHashtable { protected: template void InsertInternal(Args&&... aArgs) { + MOZ_RELEASE_ASSERT(!HasEntry()); mEntryHandle.Insert([&](PLDHashEntryHdr* entry) { new (mozilla::KnownNotNull, entry) EntryType( EntryType::KeyToPointer(mKey), std::forward(aArgs)...); @@ -354,7 +355,8 @@ class MOZ_NEEDS_NO_VTABLE_TYPE nsTHashtable { auto WithEntryHandle(KeyType aKey, F&& aFunc) -> std::invoke_result_t { return this->mTable.WithEntryHandle( - EntryType::KeyToPointer(aKey), [&aKey, &aFunc](auto entryHandle) { + EntryType::KeyToPointer(aKey), + [&aKey, &aFunc](auto entryHandle) -> decltype(auto) { return std::forward(aFunc)( EntryHandle{aKey, std::move(entryHandle)}); }); diff --git a/xpcom/tests/gtest/TestHashtables.cpp b/xpcom/tests/gtest/TestHashtables.cpp index 539926802d2f..bb206dc43064 100644 --- a/xpcom/tests/gtest/TestHashtables.cpp +++ b/xpcom/tests/gtest/TestHashtables.cpp @@ -840,6 +840,80 @@ TYPED_TEST_P(BaseHashtableTest, LookupForAdd_OrRemove) { } } +TYPED_TEST_P(BaseHashtableTest, WithEntryHandle_NoOp) { + auto table = MakeEmptyBaseHashtable(); + + table.WithEntryHandle(1, [](auto&&) {}); + + EXPECT_FALSE(table.Contains(1)); +} + +TYPED_TEST_P(BaseHashtableTest, WithEntryHandle_NotFound_OrInsert) { + auto table = MakeEmptyBaseHashtable(); + + table.WithEntryHandle(1, [](auto&& entry) { + entry.OrInsert(typename TypeParam::UserDataType( + MakeRefPtr(42))); + }); + + EXPECT_TRUE(table.Contains(1)); +} + +TYPED_TEST_P(BaseHashtableTest, WithEntryHandle_NotFound_OrInsertFrom) { + auto table = MakeEmptyBaseHashtable(); + + table.WithEntryHandle(1, [](auto&& entry) { + entry.OrInsertWith([] { + return typename TypeParam::UserDataType( + MakeRefPtr(42)); + }); + }); + + EXPECT_TRUE(table.Contains(1)); +} + +TYPED_TEST_P(BaseHashtableTest, WithEntryHandle_NotFound_OrInsertFrom_Exists) { + auto table = MakeEmptyBaseHashtable(); + + table.WithEntryHandle(1, [](auto&& entry) { + entry.OrInsertWith([] { + return typename TypeParam::UserDataType( + MakeRefPtr(42)); + }); + }); + table.WithEntryHandle(1, [](auto&& entry) { + entry.OrInsertWith([]() -> typename TypeParam::UserDataType { + ADD_FAILURE(); + return typename TypeParam::UserDataType( + MakeRefPtr(42)); + }); + }); + + EXPECT_TRUE(table.Contains(1)); +} + +TYPED_TEST_P(BaseHashtableTest, WithEntryHandle_NotFound_OrRemove) { + auto table = MakeEmptyBaseHashtable(); + + table.WithEntryHandle(1, [](auto&& entry) { entry.OrRemove(); }); + + EXPECT_FALSE(table.Contains(1)); +} + +TYPED_TEST_P(BaseHashtableTest, WithEntryHandle_NotFound_OrRemove_Exists) { + auto table = MakeEmptyBaseHashtable(); + + table.WithEntryHandle(1, [](auto&& entry) { + entry.OrInsertWith([] { + return typename TypeParam::UserDataType( + MakeRefPtr(42)); + }); + }); + table.WithEntryHandle(1, [](auto&& entry) { entry.OrRemove(); }); + + EXPECT_FALSE(table.Contains(1)); +} + TYPED_TEST_P(BaseHashtableTest, Iter) { auto table = MakeBaseHashtable(TypeParam::kExpectedAddRefCnt_Iter); @@ -924,8 +998,12 @@ REGISTER_TYPED_TEST_CASE_P( SizeOfIncludingThis, Count, IsEmpty, Get_OutputParam, Get, MaybeGet, GetOrInsert, Put, Put_Fallible, Put_Rvalue, Put_Rvalue_Fallible, Remove_OutputParam, Remove, GetAndRemove, RemoveIf, Lookup, Lookup_Remove, - LookupForAdd, LookupForAdd_OrInsert, LookupForAdd_OrRemove, Iter, ConstIter, - begin_end, cbegin_cend, Clear, ShallowSizeOfExcludingThis, + LookupForAdd, LookupForAdd_OrInsert, LookupForAdd_OrRemove, + WithEntryHandle_NoOp, WithEntryHandle_NotFound_OrInsert, + WithEntryHandle_NotFound_OrInsertFrom, + WithEntryHandle_NotFound_OrInsertFrom_Exists, + WithEntryHandle_NotFound_OrRemove, WithEntryHandle_NotFound_OrRemove_Exists, + Iter, ConstIter, begin_end, cbegin_cend, Clear, ShallowSizeOfExcludingThis, ShallowSizeOfIncludingThis, SwapElements, MarkImmutable); using BaseHashtableTestTypes = @@ -991,8 +1069,8 @@ TEST(Hashtables, DataHashtable_STLIterators) ++ci; ASSERT_TRUE(&*ci == &*otherCi); - // STL algorithms (just to check that the iterator sufficiently conforms with - // the actual syntactical requirements of those algorithms). + // 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) {}); Unused << std::find_if(