From 3a284dc19d1c77692f2faddedc674cc293d51a00 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.9 6.8 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 --- 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 425ec6ba394..cbb8772e4ba 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 524cdff7efe..e1aaccb6bed 100644 --- a/src/corelib/tools/qhash.h +++ b/src/corelib/tools/qhash.h @@ -991,12 +991,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 ba42ccb7d79..cdcb293a7e7 100644 --- a/tests/auto/corelib/tools/qset/tst_qset.cpp +++ b/tests/auto/corelib/tools/qset/tst_qset.cpp @@ -402,17 +402,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));