From 12f39bb2ab0e2bbf70c538af7b36014024b5901a Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Thu, 2 Jan 2025 09:22:23 +0100 Subject: [PATCH] QSet: check that unite() and intersect() prefer elements from *this In Qt versions 5.0..6.7, at least, this used to be the implemented behavior for unite(). For intersect(), it was implemented up to and excl. 5.2. Even the documentation states as much: intersect(): Removes all items from _this set_ that are not contained in the other set. unite(): Each item in the other set that isn't already in this set is inserted into this set. Add checks that the functions behave as documented (hint: they no longer do, since 6.8 (unite()) and 5.2 (intersect()), resp.), this being the only correct way to implement these functions (items in sets may be equivalent, but not identical; it is important that the set operations work in a manner consistent with insert(), to meet user's expectation of how these functions work (unite() just inserts() the rhs, intersect() removes if !rhs.contains). Note that QSet, unlike other Qt associative containers, actually has STL-style insertion behavior (insert() doesn't overwrite). subtract() is the only one that's still true to its docs. A test for this function will be added in a follow-up commit - eventually. In anticipation of adding rvalue-other overloads of at least unite(), add a test with that, too. Pick-to: 6.8 6.5 6.2 5.15 Task-number: QTBUG-132500 Task-number: QTBUG-132536 Change-Id: Id309ab5192e6d1c9bbeef496cbd7116d306eaae8 Reviewed-by: Allan Sandfeld Jensen Reviewed-by: Edward Welbourne (cherry picked from commit 0d4fd5c545b47966c56ed0b9eb9b5c0a8f75c02a) Reviewed-by: Qt Cherry-pick Bot --- tests/auto/corelib/tools/qset/tst_qset.cpp | 139 +++++++++++++++++++++ 1 file changed, 139 insertions(+) diff --git a/tests/auto/corelib/tools/qset/tst_qset.cpp b/tests/auto/corelib/tools/qset/tst_qset.cpp index cdcb293a7e7..74e39da0427 100644 --- a/tests/auto/corelib/tools/qset/tst_qset.cpp +++ b/tests/auto/corelib/tools/qset/tst_qset.cpp @@ -52,6 +52,12 @@ private slots: void intersects(); void find(); void values(); + void setOperationsPickEquivalentElementsFromLHSContainer_data(); + void setOperationsPickEquivalentElementsFromLHSContainer(); + +private: + template + void setOperationsPickEquivalentElementsFromLHSContainer_impl(); }; struct IdentityTracker { @@ -1217,6 +1223,139 @@ void tst_QSet::values() QCOMPARE(sorted(set.values()), QList({ 1, 2, 10 })); } +void tst_QSet::setOperationsPickEquivalentElementsFromLHSContainer_data() +{ + QTest::addColumn("lhsShared"); + QTest::addColumn("rhsShared"); + QTest::addColumn("rhsRValue"); + + for (bool lhs : {true, false}) { + for (bool rhs : {true, false}) { + for (bool rv : {true, false}) { + QTest::addRow("lhs %sshared, rhs %sshared %cvalue", + lhs ? "" : "not ", rhs ? "" : "not ", rv ? 'r' : 'l') + << lhs << rhs << rv; + } + } + } +} + +void tst_QSet::setOperationsPickEquivalentElementsFromLHSContainer() +{ + QFETCH(const bool, rhsRValue); + + if (rhsRValue) + setOperationsPickEquivalentElementsFromLHSContainer_impl(); + else + setOperationsPickEquivalentElementsFromLHSContainer_impl(); +} + +template +void tst_QSet::setOperationsPickEquivalentElementsFromLHSContainer_impl() +{ + QFETCH(const bool, lhsShared); + QFETCH(const bool, rhsShared); + + // + // GIVEN: Two equivalent QSets: + // + constexpr IdentityTracker OneL{1, 1}, TwoL{2, 1}, ThreeL{3, 1}; + constexpr IdentityTracker OneR{1, 2}, TwoR{2, 2}, ThreeR{3, 2}; + + // by construction: + QCOMPARE(OneL, OneR); + QCOMPARE(qHash(OneL), qHash(OneR)); + + const QSet lhs = {OneL, TwoL}; + const QSet rhs = {OneR, TwoR, ThreeR}; + + auto maybeDetach = [](auto set, bool shared) { + if (!shared) + set.detach(); + return set; + }; + + { + // + // WHEN: intersect()ing the two + // + auto lhsCopy = maybeDetach(lhs, lhsShared); + auto rhsCopy = maybeDetach(rhs, rhsShared); + + if constexpr (RhsRValue) { + lhsCopy.intersect(maybeDetach(rhs, rhsShared)); + rhsCopy.intersect(maybeDetach(lhs, lhsShared)); + } else { + lhsCopy.intersect(rhs); + rhsCopy.intersect(lhs); + } + + // + // THEN: equivalent elements are picked from the LHS container: + // (unlike other Qt containers, QSet's insertion behavior is STL-compliant): + // + QVERIFY(lhsCopy.contains(OneL)); + QCOMPARE(lhsCopy.find(OneL)->id, OneL.id); + + QVERIFY(lhsCopy.contains(TwoL)); + QCOMPARE(lhsCopy.find(TwoL)->id, TwoL.id); + + QVERIFY(!lhsCopy.contains(ThreeR)); + + + QVERIFY(rhsCopy.contains(OneR)); + QEXPECT_FAIL("", "QTBUG-132536", Continue); + QCOMPARE(rhsCopy.find(OneR)->id, OneR.id); + + QVERIFY(rhsCopy.contains(TwoR)); + QEXPECT_FAIL("", "QTBUG-132536", Continue); + QCOMPARE(rhsCopy.find(TwoR)->id, TwoR.id); + + QVERIFY(!rhsCopy.contains(ThreeR)); + } + + { + // + // WHEN: unite()ing the two + // + auto lhsCopy = maybeDetach(lhs, lhsShared); + auto rhsCopy = maybeDetach(rhs, rhsShared); + + if constexpr (RhsRValue) { + lhsCopy.unite(maybeDetach(rhs, rhsShared)); + rhsCopy.unite(maybeDetach(lhs, lhsShared)); + } else { + lhsCopy.unite(rhs); + rhsCopy.unite(lhs); + } + + // + // THEN: equivalent elements are picked from the LHS container: + // (unlike other Qt containers, QSet's insertion behavior is STL-compliant): + // + QVERIFY(lhsCopy.contains(OneL)); + QEXPECT_FAIL("", "QTBUG-132500", Continue); + QCOMPARE(lhsCopy.find(OneL)->id, OneL.id); + + QVERIFY(lhsCopy.contains(TwoL)); + QEXPECT_FAIL("", "QTBUG-132500", Continue); + QCOMPARE(lhsCopy.find(TwoL)->id, TwoL.id); + + QVERIFY(lhsCopy.contains(ThreeL)); + QCOMPARE(lhsCopy.find(ThreeL)->id, ThreeR.id); // element was not contained in lhs + + + QVERIFY(rhsCopy.contains(OneR)); + QCOMPARE(rhsCopy.find(OneR)->id, OneR.id); + + QVERIFY(rhsCopy.contains(TwoR)); + QCOMPARE(rhsCopy.find(TwoR)->id, TwoR.id); + + QVERIFY(rhsCopy.contains(ThreeR)); + QCOMPARE(rhsCopy.find(ThreeR)->id, ThreeR.id); + } +} + QTEST_APPLESS_MAIN(tst_QSet) #include "tst_qset.moc"