From 4299115fe62f23665692c1eb8cf290c909b888b5 Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Tue, 14 Jan 2025 10:10:32 +0100 Subject: [PATCH] QSet: don't detach in remove()/removeIf() if nothing is being removed QSet::remove() calls QHash::remove(), which unconditionally detaches, so fixing the detach there, by pulling the isUnused() check to before detach(), fixes it for both. For removeIf(), the old code used begin()/end() for iteration, detaching unconditionally, even if nothing was removed in the end. In this case, fix by using cbegin()/cend(). That delays the detach() until the first erase(), where it belongs. The end() iterator of QHash (and therefore QSet) are stateless sentinels in all but name, so we can continue to cache end(). Add a code comment to that effect. Amends 62dad9be9e172597c56a970202299563aa04918e (for removeIf()) and 5b7c3e31b538376f2b4733bd868b5875b504cdb3 (for remove()). [ChangeLog][QtCore][QSet] remove() and removeIf() no longer unconditionally detach, but only if something is actually being removed. [ChangeLog][QtCore][QHash] remove() no longer unconditionally detaches, but only if something is actually being removed. Pick-to: 6.5 Task-number: QTBUG-106179 Fixes: QTBUG-132831 Change-Id: I807577eafa1be478b0a2da45cf8c44936d5e48ed Reviewed-by: Edward Welbourne Reviewed-by: Thiago Macieira Reviewed-by: Allan Sandfeld Jensen (cherry picked from commit 3a284dc19d1c77692f2faddedc674cc293d51a00) Reviewed-by: Qt Cherry-pick Bot (cherry picked from commit 605a20b2624dac1bfc4a4cb683700db5026bdbe8) --- src/corelib/tools/qcontainertools_impl.h | 5 +++-- src/corelib/tools/qhash.h | 5 +++-- tests/auto/corelib/tools/qset/tst_qset.cpp | 8 -------- 3 files changed, 6 insertions(+), 12 deletions(-) diff --git a/src/corelib/tools/qcontainertools_impl.h b/src/corelib/tools/qcontainertools_impl.h index 57c9c29835..7b3c000c3c 100644 --- a/src/corelib/tools/qcontainertools_impl.h +++ b/src/corelib/tools/qcontainertools_impl.h @@ -398,12 +398,13 @@ template qsizetype qset_erase_if(QSet &set, Predicate &pred) { qsizetype result = 0; - auto it = set.begin(); - const auto e = set.end(); + auto it = set.cbegin(); + auto e = set.cend(); // stable across detach (QHash::end() is a stateless sentinel)... while (it != e) { if (pred(*it)) { ++result; it = set.erase(it); + e = set.cend(); // ...but re-set nonetheless, in case at some point it won't be } else { ++it; } diff --git a/src/corelib/tools/qhash.h b/src/corelib/tools/qhash.h index 3a3e1842b9..f2fa0aa448 100644 --- a/src/corelib/tools/qhash.h +++ b/src/corelib/tools/qhash.h @@ -975,12 +975,13 @@ private: if (isEmpty()) // prevents detaching shared null return false; auto it = d->findBucket(key); + if (it.isUnused()) + return false; + size_t bucket = it.toBucketIndex(d); detach(); it = typename Data::Bucket(d, bucket); // reattach in case of detach - if (it.isUnused()) - return false; d->erase(it); return true; } diff --git a/tests/auto/corelib/tools/qset/tst_qset.cpp b/tests/auto/corelib/tools/qset/tst_qset.cpp index fca39e9be1..5c86d496cb 100644 --- a/tests/auto/corelib/tools/qset/tst_qset.cpp +++ b/tests/auto/corelib/tools/qset/tst_qset.cpp @@ -379,17 +379,9 @@ void tst_QSet::removeOnlyDetachesIfSomethingGetsRemoved() QVERIFY(!copy.isDetached()); QVERIFY(!copy.remove(42)); - QEXPECT_FAIL("", "QTBUG-132831", Continue); - QVERIFY(!copy.isDetached()); - - copy = set; QVERIFY(!copy.isDetached()); QCOMPARE(copy.removeIf([] (auto) { return false; }), 0); - QEXPECT_FAIL("", "QTBUG-132831", Continue); - QVERIFY(!copy.isDetached()); - - copy = set; QVERIFY(!copy.isDetached()); QVERIFY(copy.remove(4));