Unbreak QSet::intersect()

The selection of which set to iterate over and which one to remove
from based on their relative size violates the function's
documentation, which clearly states that items are removed from *this,
and not from `other`, so the result must never contain any elements
from `other`.

Amends 4f2c96eaa8bfa4d8a6dfb92096e4e4030d0cdea7. Instead of reverting
to the gruesome old code with the forced detach-just-to-remove copies,
distinguish four cases:

- if the two sets are shallow copies of each other, then their
  intersection is *this

- otherwise, if either set is empty, clear() *this. This is required
  for one of the tests that 29017f1395b1bc52e60760fa58c92f6fa4ee4f60
  added to succeed.

- otherwise, if *this is detached, perform the operation in-place,
  using removeIf()

- otherwise, create a new set and move-assign to *this to avoid
  detaching just to remove something again. In this case, we can
  continue to iterate over the smaller set, but we need to keep
  picking elements from LHS into the result.

[ChangeLog][QtCore][QSet] Fixed a regression (introduced for Qt 5.2)
in intersect() that caused equivalent elements of `*this` to be
overwritten by elements of `other` if `other.size()` was larger than
`this->size()`.

Not picking to 5.15, as users will have likely adjusted their code to
the buggy behavior, and because removeIf() isn't available there.

Pick-to: 6.9 6.8 6.5
Fixes: QTBUG-132536
Task-number: QTBUG-106179
Change-Id: Idfa17c3b3589c4eacec27259fc01df6aeaa6c45f
Reviewed-by: Øystein Heskestad <oystein.heskestad@qt.io>
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>
This commit is contained in:
Marc Mutz 2025-01-02 08:31:21 +01:00
parent 2d1b302867
commit 162015e9c6
2 changed files with 42 additions and 14 deletions

View File

@ -209,6 +209,8 @@ public:
QList<T> values() const;
private:
static inline QSet intersected_helper(const QSet &lhs, const QSet &rhs);
Hash q_hash;
};
@ -242,23 +244,51 @@ Q_INLINE_TEMPLATE QSet<T> &QSet<T>::unite(const QSet<T> &other)
template <class T>
Q_INLINE_TEMPLATE QSet<T> &QSet<T>::intersect(const QSet<T> &other)
{
QSet<T> copy1;
QSet<T> copy2;
if (size() <= other.size()) {
copy1 = *this;
copy2 = other;
if (q_hash.isSharedWith(other.q_hash)) {
// nothing to do
} else if (isEmpty() || other.isEmpty()) {
// any set intersected with the empty set is the empty set
clear();
} else if (q_hash.isDetached()) {
// do it in-place:
removeIf([&other] (const T &e) { return !other.contains(e); });
} else {
copy1 = other;
copy2 = *this;
*this = copy1;
}
for (const auto &e : std::as_const(copy1)) {
if (!copy2.contains(e))
remove(e);
// don't detach *this just to remove some items; create a new set
*this = intersected_helper(*this, other);
}
return *this;
}
template <class T>
// static
auto QSet<T>::intersected_helper(const QSet &lhs, const QSet &rhs) -> QSet
{
QSet r;
const auto l_size = lhs.size();
const auto r_size = rhs.size();
r.reserve((std::min)(l_size, r_size));
// Iterate the smaller of the two sets, but always take from lhs, for
// consistency with insert():
if (l_size <= r_size) {
// lhs is not larger
for (const auto &e : lhs) {
if (rhs.contains(e))
r.insert(e);
}
} else {
// rhs is smaller
for (const auto &e : rhs) {
if (const auto it = lhs.find(e); it != lhs.end())
r.insert(*it);
}
}
return r;
}
template <class T>
Q_INLINE_TEMPLATE bool QSet<T>::intersects(const QSet<T> &other) const
{

View File

@ -1304,11 +1304,9 @@ void tst_QSet::setOperationsPickEquivalentElementsFromLHSContainer_impl()
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));