From d9ad2251d9fff85a18ce5afc62bcb1230cd2820d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= Date: Wed, 31 Jan 2024 14:26:27 +0100 Subject: [PATCH] Add QHash::tryEmplace/try_emplace Like QHash::emplace, but since it returns a bool (inserted/not inserted) we finally have a way to avoid the size(), emplace()/insert()/op[], size() pattern. We also provide try_emplace (also with the hint argument overloads even though we completely ignore the hint), for general compatibility with the rest of the standard library. And for that reason it must also use key_value_iterator instead of our usual iterator. [ChangeLog][QtCore][QHash] Added tryEmplace(). [ChangeLog][QtCore][QHash] Added try_emplace() for compatibility with the standard library. Fixes: QTBUG-130258 Change-Id: Ie14786591ad6e9805e120e11fa01f69349ed4528 Reviewed-by: Thiago Macieira Reviewed-by: Marc Mutz --- src/corelib/tools/qhash.cpp | 108 ++++++++++++++++ src/corelib/tools/qhash.h | 118 +++++++++++++++--- .../tst_containerapisymmetry.cpp | 35 ++++++ tests/auto/corelib/tools/qhash/tst_qhash.cpp | 100 +++++++++++++++ 4 files changed, 346 insertions(+), 15 deletions(-) diff --git a/src/corelib/tools/qhash.cpp b/src/corelib/tools/qhash.cpp index bd8deb34703..6c4a5a1a806 100644 --- a/src/corelib/tools/qhash.cpp +++ b/src/corelib/tools/qhash.cpp @@ -2297,6 +2297,114 @@ size_t qHash(long double key, size_t seed) noexcept \include qhash.cpp qhash-iterator-invalidation-func-desc */ +/*! + \class QHash::TryEmplaceResult + \inmodule QtCore + \since 6.9 + \ingroup tools + \brief The TryEmplaceResult class is used to represent the result of a tryEmplace operation. + + The \c{TryEmplaceResult} class is used in QHash to represent the result + of a tryEmplace operation. It holds an \l{iterator} to the newly + created item, or to the pre-existing item that prevented the insertion, and + a boolean, \l{inserted}, denoting whether the insertion took place. + + \sa QHash, QHash::tryEmplace() +*/ + +/*! + \variable QHash::TryEmplaceResult::iterator + + Holds the iterator to the newly inserted element, or the element that + prevented the insertion. +*/ + +/*! + \variable QHash::TryEmplaceResult::inserted + + This value is \c{false} if there was already an entry with the same key. +*/ + +/*! + \fn template template QHash::TryEmplaceResult QHash::tryEmplace(const Key &key, Args &&...args) + \fn template template QHash::TryEmplaceResult QHash::tryEmplace(Key &&key, Args &&...args) + \fn template template = true, if_key_constructible_from = true> QHash::TryEmplaceResult QHash::tryEmplace(K &&key, Args &&...args) + \since 6.9 + + Inserts a new item with the \a key and a value constructed from \a args. + If an item with \a key already exists, no insertion takes place. + + Returns an instance of \l{TryEmplaceResult}, a structure that holds an + \l{QHash::TryEmplaceResult::}{iterator} to the newly created item, or + to the pre-existing item that prevented the insertion, and a boolean, + \l{QHash::TryEmplaceResult::}{inserted}, denoting whether the insertion + took place. + + For example, this can be used to avoid the pattern of comparing old and + new size or double-lookups. Where you might previously have written code like: + + \code + QHash hash; + // [...] + int myKey = getKey(); + qsizetype oldSize = hash.size(); + MyType &elem = hash[myKey]; + if (oldSize != hash.size()) // Size changed: new element! + initialize(elem); + // [use elem...] + \endcode + + You can instead write: + + \code + QHash hash; + // [...] + int myKey = getKey(); + auto result = hash.tryEmplace(myKey); + if (result.inserted) // New element! + initialize(*result.iterator); + // [use result.iterator...] + \endcode + + \sa emplace() +*/ + +/*! + \fn template template = true, if_key_constructible_from = true> QHash::try_emplace(const_iterator hint, K &&key, Args &&...args) + \fn template template iterator QHash::try_emplace(const_iterator hint, const Key &key, Args &&...args) + \fn template template iterator QHash::try_emplace(const_iterator hint, Key &&key, Args &&...args) + \since 6.9 + + Inserts a new item with the \a key and a value constructed from \a args. + If an item with \a key already exists, no insertion takes place. + + Returns the iterator of the inserted item, or to the item that prevented the + insertion. + + \a hint is ignored. + + These functions are provided for compatibility with the standard library. + + \sa emplace(), tryEmplace() +*/ + +/*! + \fn template template std::pair QHash::try_emplace(const Key &key, Args &&...args) + \fn template template std::pair QHash::try_emplace(Key &&key, Args &&...args) + \fn template template = true, if_key_constructible_from = true> QHash::try_emplace(K &&key, Args &&...args) + \since 6.9 + + Inserts a new item with the \a key and a value constructed from \a args. + If an item with \a key already exists, no insertion takes place. + + Returns a pair consisting of an iterator to the inserted item (or to the + item that prevented the insertion), and a bool denoting whether the + insertion took place. + + These functions are provided for compatibility with the standard library. + + \sa emplace(), tryEmplace() +*/ /*! \fn template void QHash::insert(const QHash &other) \since 5.15 diff --git a/src/corelib/tools/qhash.h b/src/corelib/tools/qhash.h index 8e87fabfb01..3f6aba3f4e6 100644 --- a/src/corelib/tools/qhash.h +++ b/src/corelib/tools/qhash.h @@ -15,6 +15,7 @@ #include #include // for std::hash +#include class tst_QHash; // for befriending @@ -812,7 +813,12 @@ struct iterator { { return !(*this == other); } }; - +template +using HeterogenousConstructProxy = std::conditional_t< + std::is_same_v>, + KeyArgument, // HashKey == KeyArg w/ potential modifiers, so we keep modifiers + HashKey + >; } // namespace QHashPrivate @@ -1090,21 +1096,9 @@ public: T &operator[](const Key &key) { - return operatorIndexImpl(key); - } -private: - template T &operatorIndexImpl(const K &key) - { - const auto copy = isDetached() ? QHash() : *this; // keep 'key' alive across the detach - detach(); - auto result = d->findOrInsert(key); - Q_ASSERT(!result.it.atEnd()); - if (!result.initialized) - Node::createInPlace(result.it.node(), Key(key), T()); - return result.it.node()->value; + return *tryEmplace(key).iterator; } -public: const T operator[](const Key &key) const noexcept { return value(key); @@ -1255,6 +1249,12 @@ public: auto asKeyValueRange() && { return QtPrivate::QKeyValueRange(std::move(*this)); } auto asKeyValueRange() const && { return QtPrivate::QKeyValueRange(std::move(*this)); } + struct TryEmplaceResult + { + QHash::iterator iterator; + bool inserted; + }; + iterator erase(const_iterator it) { Q_ASSERT(it != constEnd()); @@ -1366,6 +1366,77 @@ public: return emplace_helper(std::move(key), std::forward(args)...); } + template + TryEmplaceResult tryEmplace(const Key &key, Args &&...args) + { + return tryEmplace_impl(key, std::forward(args)...); + } + template + TryEmplaceResult tryEmplace(Key &&key, Args &&...args) + { + return tryEmplace_impl(std::move(key), std::forward(args)...); + } + + template + std::pair try_emplace(const Key &key, Args &&...args) + { + auto r = tryEmplace_impl(key, std::forward(args)...); + return {key_value_iterator(r.iterator), r.inserted}; + } + template + std::pair try_emplace(Key &&key, Args &&...args) + { + auto r = tryEmplace_impl(std::move(key), std::forward(args)...); + return {key_value_iterator(r.iterator), r.inserted}; + } + template + key_value_iterator try_emplace(const_iterator /*hint*/, const Key &key, Args &&...args) + { + auto r = tryEmplace_impl(key, std::forward(args)...); + return key_value_iterator(r.iterator); + } + template + key_value_iterator try_emplace(const_iterator /*hint*/, Key &&key, Args &&...args) + { + auto r = tryEmplace_impl(std::move(key), std::forward(args)...); + return key_value_iterator(r.iterator); + } + +private: + template + TryEmplaceResult tryEmplace_impl(K &&key, Args &&...args) + { + if (!d) + detach(); + QHash detachGuard; + + typename Data::Bucket bucket = d->findBucket(key); + const bool shouldInsert = bucket.isUnused(); + + // Even if we don't insert we may have to detach because we are + // returning a non-const iterator: + if (!isDetached() || (shouldInsert && d->shouldGrow())) { + detachGuard = *this; + const bool resized = shouldInsert && d->shouldGrow(); + const size_t bucketIndex = bucket.toBucketIndex(d); + // Like reserve(), but unconditionally detaching if no need to grow: + if (isDetached()) + d->rehash(d->size + 1); + else + d = Data::detached(d, d->size + (shouldInsert ? 1 : 0)); + bucket = resized ? d->findBucket(key) : typename Data::Bucket(d, bucketIndex); + } + if (shouldInsert) { + Node *n = bucket.insert(); + using ConstructProxy = typename QHashPrivate::HeterogenousConstructProxy; + Node::createInPlace(n, ConstructProxy(std::forward(key)), + std::forward(args)...); + ++d->size; + } + return {iterator(bucket.toIterator(d)), shouldInsert}; + } +public: + float load_factor() const noexcept { return d ? d->loadFactor() : 0; } static float max_load_factor() noexcept { return 0.5; } size_t bucket_count() const noexcept { return d ? d->numBuckets : 0; } @@ -1431,7 +1502,7 @@ public: template = true, if_key_constructible_from = true> T &operator[](const K &key) { - return operatorIndexImpl(key); + return *tryEmplace(key).iterator; } template = true> const T operator[](const K &key) const noexcept @@ -1465,6 +1536,23 @@ public: { return find(key); } + template = true, if_key_constructible_from = true> + TryEmplaceResult tryEmplace(K &&key, Args &&...args) + { + return tryEmplace_impl(std::forward(key), std::forward(args)...); + } + template = true, if_key_constructible_from = true> + std::pair try_emplace(K &&key, Args &&...args) + { + auto r = tryEmplace_impl(std::forward(key), std::forward(args)...); + return { key_value_iterator(r.iterator), r.inserted }; + } + template = true, if_key_constructible_from = true> + key_value_iterator try_emplace(const_iterator /*hint*/, K &&key, Args &&...args) + { + auto r = tryEmplace_impl(std::forward(key), std::forward(args)...); + return key_value_iterator(r.iterator); + } }; diff --git a/tests/auto/corelib/tools/containerapisymmetry/tst_containerapisymmetry.cpp b/tests/auto/corelib/tools/containerapisymmetry/tst_containerapisymmetry.cpp index beefa2c8a9a..8c11e048ad5 100644 --- a/tests/auto/corelib/tools/containerapisymmetry/tst_containerapisymmetry.cpp +++ b/tests/auto/corelib/tools/containerapisymmetry/tst_containerapisymmetry.cpp @@ -447,6 +447,14 @@ private Q_SLOTS: void opEqNaN_QSet_Float() { opEqNaN_impl>(); } void opEqNaN_QSet_Float16() { opEqNaN_impl>(); } void opEqNaN_QSet_Double() { opEqNaN_impl>(); } + +private: + template + void try_emplace_impl() const; + +private Q_SLOTS: + void try_emplace_QHash() { try_emplace_impl>(); } + void try_emplace_unordered_map() { try_emplace_impl>(); } }; void tst_ContainerApiSymmetry::init() @@ -1306,5 +1314,32 @@ void tst_ContainerApiSymmetry::opEqNaN_impl() const QCOMPARE_NE(lhs, rhs); } +template +void tst_ContainerApiSymmetry::try_emplace_impl() const +{ + using K = typename Container::key_type; + using V = typename Container::mapped_type; + Container c; + auto p = c.try_emplace(K(), V()); + QVERIFY(p.second); + QCOMPARE(p.first->first, K()); + QCOMPARE(p.first->second, V()); + + auto it = c.try_emplace(c.begin(), K(), V()); + QCOMPARE(it->first, K()); + QCOMPARE(it->second, V()); + + K k{}; + V v{}; + p = c.try_emplace(k, v); + QVERIFY(!p.second); + QCOMPARE(p.first->first, K()); + QCOMPARE(p.first->second, V()); + + it = c.try_emplace(c.begin(), k, v); + QCOMPARE(it->first, K()); + QCOMPARE(it->second, V()); +} + QTEST_APPLESS_MAIN(tst_ContainerApiSymmetry) #include "tst_containerapisymmetry.moc" diff --git a/tests/auto/corelib/tools/qhash/tst_qhash.cpp b/tests/auto/corelib/tools/qhash/tst_qhash.cpp index 59e8ed6b586..baa70dbd7ee 100644 --- a/tests/auto/corelib/tools/qhash/tst_qhash.cpp +++ b/tests/auto/corelib/tools/qhash/tst_qhash.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include @@ -81,6 +82,7 @@ private slots: void multiHashStoresInReverseInsertionOrder(); void emplace(); + void tryEmplace(); void badHashFunction(); void hashOfHash(); @@ -2829,6 +2831,104 @@ void tst_QHash::emplace() } } +void tst_QHash::tryEmplace() +{ + { + QHash hash; + QHash::TryEmplaceResult r = hash.tryEmplace(0, 0); + QCOMPARE_NE(r.iterator, hash.end()); + QCOMPARE(hash.size(), 1); + QCOMPARE(hash[0], 0); + QVERIFY(r.inserted); + QCOMPARE(*r.iterator, 0); + int cref = 0; + r = hash.tryEmplace(cref, 1); + QCOMPARE_NE(r.iterator, hash.end()); + QCOMPARE(hash.size(), 1); + QCOMPARE(hash[0], 0); + QVERIFY(!r.inserted); + QCOMPARE(*r.iterator, 0); + + auto [iterator, inserted] = hash.tryEmplace(1, 1); + QCOMPARE_NE(iterator, hash.end()); + QCOMPARE(hash.size(), 2); + QCOMPARE(hash[1], 1); + QVERIFY(inserted); + QCOMPARE(*iterator, 1); + + auto [it, inserted2] = hash.try_emplace(cref, 1); + QCOMPARE_NE(it, hash.keyValueEnd()); + QCOMPARE(hash.size(), 2); + QCOMPARE(hash[0], 0); + QVERIFY(!inserted2); + QCOMPARE(it->second, 0); + } + { + // Make sure any kind of resize is properly handled + QHash hash; + hash.reserve(1); + const qsizetype end = hash.capacity() + 2; + auto [it, inserted] = hash.try_emplace(0, 0); + for (int i = 1; i < end; ++i) { + QHash copy = hash; + + // Test pure detach, no insert, works on any capacity: + std::tie(it, inserted) = hash.try_emplace(i - 1, i); + QCOMPARE_NE(it, hash.keyValueEnd()); + QVERIFY(!inserted); + QCOMPARE(it->second, i - 1); + QVERIFY(!hash.isSharedWith(copy)); + + copy = hash; + // And when an insertion _does_ happen: + std::tie(it, inserted) = hash.try_emplace(i, i); + QVERIFY(inserted); + QCOMPARE(it->second, i); + } + QCOMPARE(hash.size(), end); + QCOMPARE_GT(hash.capacity(), end); + } + { + // Heterogeneous key + QHash hash; + auto [it, inserted] = hash.try_emplace(HeterogeneousHashingType{u"Hello World"_s}, 242); + QCOMPARE_NE(it, hash.keyValueEnd()); + QVERIFY(inserted); + QCOMPARE(it->second, 242); + QCOMPARE(hash.size(), 1); + QCOMPARE(it->first, u"Hello World"_s); + + auto r = hash.tryEmplace(HeterogeneousHashingType{u"Uniquely"_s}, 1); + QCOMPARE_NE(r.iterator, hash.end()); + QVERIFY(r.inserted); + QCOMPARE(*r.iterator, 1); + QCOMPARE(hash.size(), 2); + QCOMPARE(r.iterator.key(), u"Uniquely"_s); + } + // Overloads taking hint: + { + QHash hash; + auto it = hash.try_emplace(hash.end(), HeterogeneousHashingType{u"Hello World"_s}, 242); + QCOMPARE_NE(it, hash.keyValueEnd()); + QCOMPARE(it->second, 242); + QCOMPARE(hash.size(), 1); + QCOMPARE(it->first, u"Hello World"_s); + + it = hash.try_emplace(hash.begin(), u"Uniquely"_s, 1); + QCOMPARE_NE(it, hash.keyValueEnd()); + QCOMPARE(it->second, 1); + QCOMPARE(hash.size(), 2); + QCOMPARE(it->first, u"Uniquely"_s); + + QString cref = u"Uniquely"_s; + it = hash.try_emplace(hash.end(), cref, 16); + QCOMPARE_NE(it, hash.keyValueEnd()); + QCOMPARE(it->second, 1); + QCOMPARE(hash.size(), 2); + QCOMPARE(it->first, u"Uniquely"_s); + } +} + struct BadKey { int k; BadKey(int i) : k(i) {}