whytofly
/
qt6-bb10
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

QMultiMap: fix remove(Key, T) when key/value belong to the map 

Just like any other container, it's legitimate for the user to
pass key/values belonging to the same container.
Q(Multi)Map::remove(Key) are already safe (either they call
erase() directly on std::(multi)map, where it does the right thing,
or they skip elements while detaching).
However, QMultiMap::remove(Key, T) wasn't safe in this regard
(the implementation is hand rolled), so take copies before start
erasing.

Change-Id: I87767d608b83216a6ff264fb6c8f145fdb5934f8
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
bb10
Giuseppe D'Angelo 2020-08-17 17:31:44 +02:00
parent 041f655c01
commit 2ba1d540e6
2 changed files with 180 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
src/corelib/tools/qmap.h
View File

@ -910,14 +910,20 @@ public:
// TODO: improve. Copy over only the elements not to be removed.
detach();
// key and value may belong to this map. As such, we need to copy
// them to ensure they stay valid througout the iteration below
// (which may destroy them)
const Key keyCopy = key;
const T valueCopy = value;
size_type result = 0;
const auto &keyCompare = d->m.key_comp();
auto i = d->m.find(key);
auto i = d->m.find(keyCopy);
const auto e = d->m.end();
while (i != e && !keyCompare(key, i->first)) {
if (i->second == value) {
while (i != e && !keyCompare(keyCopy, i->first)) {
if (i->second == valueCopy) {
i = d->m.erase(i);
++result;
} else {

171
tests/auto/corelib/tools/qmap/tst_qmap.cpp
View File

@ -77,6 +77,7 @@ private slots:
void testInsertWithHint();
void testInsertMultiWithHint();
void eraseValidIteratorOnSharedMap();
void removeElementsInMap();
};
struct IdentityTracker {
@ -1649,5 +1650,175 @@ void tst_QMap::eraseValidIteratorOnSharedMap()
QCOMPARE(ms3.size(), 3);
}
void tst_QMap::removeElementsInMap()
{
// A class that causes an almost certain crash if its operator< is
// called on a destroyed object
struct SharedInt {
QSharedPointer<int> m_int;
explicit SharedInt(int i) : m_int(QSharedPointer<int>::create(i)) {}
bool operator<(const SharedInt &other) const { return *m_int < *other.m_int; }
};
{
QMap<SharedInt, int> map {
{ SharedInt(1), 1 },
{ SharedInt(2), 2 },
{ SharedInt(3), 3 },
{ SharedInt(4), 4 },
{ SharedInt(5), 5 },
};
QCOMPARE(map.size(), 5);
map.remove(SharedInt(1));
QCOMPARE(map.size(), 4);
map.remove(SharedInt(-1));
QCOMPARE(map.size(), 4);
QMap<SharedInt, int> map2 = map;
QCOMPARE(map.size(), 4);
QCOMPARE(map2.size(), 4);
map.remove(SharedInt(3));
QCOMPARE(map.size(), 3);
QCOMPARE(map2.size(), 4);
map.remove(SharedInt(-1));
QCOMPARE(map.size(), 3);
QCOMPARE(map2.size(), 4);
map = map2;
QCOMPARE(map.size(), 4);
QCOMPARE(map2.size(), 4);
map.remove(SharedInt(-1));
QCOMPARE(map.size(), 4);
QCOMPARE(map2.size(), 4);
map.remove(map.firstKey());
QCOMPARE(map.size(), 3);
QCOMPARE(map2.size(), 4);
map.remove(map.lastKey());
QCOMPARE(map.size(), 2);
QCOMPARE(map2.size(), 4);
map = map2;
QCOMPARE(map.size(), 4);
QCOMPARE(map2.size(), 4);
auto size = map.size();
for (auto it = map.begin(); it != map.end(); ) {
const auto oldIt = it++;
size -= map.remove(oldIt.key());
QCOMPARE(map.size(), size);
QCOMPARE(map2.size(), 4);
}
QCOMPARE(map.size(), 0);
QCOMPARE(map2.size(), 4);
}
{
QMultiMap<SharedInt, int> multimap {
{ SharedInt(1), 10 },
{ SharedInt(1), 11 },
{ SharedInt(2), 2 },
{ SharedInt(3), 30 },
{ SharedInt(3), 31 },
{ SharedInt(3), 32 },
{ SharedInt(4), 4 },
{ SharedInt(5), 5 },
{ SharedInt(6), 60 },
{ SharedInt(6), 61 },
{ SharedInt(6), 60 },
{ SharedInt(6), 62 },
{ SharedInt(6), 60 },
{ SharedInt(7), 7 },
};
QCOMPARE(multimap.size(), 14);
multimap.remove(SharedInt(1));
QCOMPARE(multimap.size(), 12);
multimap.remove(SharedInt(-1));
QCOMPARE(multimap.size(), 12);
QMultiMap<SharedInt, int> multimap2 = multimap;
QCOMPARE(multimap.size(), 12);
QCOMPARE(multimap2.size(), 12);
multimap.remove(SharedInt(3));
QCOMPARE(multimap.size(), 9);
QCOMPARE(multimap2.size(), 12);
multimap.remove(SharedInt(4));
QCOMPARE(multimap.size(), 8);
QCOMPARE(multimap2.size(), 12);
multimap.remove(SharedInt(-1));
QCOMPARE(multimap.size(), 8);
QCOMPARE(multimap2.size(), 12);
multimap = multimap2;
QCOMPARE(multimap.size(), 12);
QCOMPARE(multimap2.size(), 12);
multimap.remove(SharedInt(-1));
QCOMPARE(multimap.size(), 12);
QCOMPARE(multimap2.size(), 12);
multimap.remove(SharedInt(6), 60);
QCOMPARE(multimap.size(), 9);
QCOMPARE(multimap2.size(), 12);
multimap = multimap2;
QCOMPARE(multimap.size(), 12);
QCOMPARE(multimap2.size(), 12);
multimap.remove(SharedInt(6), 62);
QCOMPARE(multimap.size(), 11);
QCOMPARE(multimap2.size(), 12);
multimap.remove(multimap.firstKey());
QCOMPARE(multimap.size(), 10);
QCOMPARE(multimap2.size(), 12);
multimap.remove(multimap.lastKey());
QCOMPARE(multimap.size(), 9);
QCOMPARE(multimap2.size(), 12);
multimap = multimap2;
QCOMPARE(multimap.size(), 12);
QCOMPARE(multimap2.size(), 12);
auto itFor6 = multimap.find(SharedInt(6));
QVERIFY(itFor6 != multimap.end());
QCOMPARE(itFor6.value(), 60);
multimap.remove(itFor6.key(), itFor6.value());
QCOMPARE(multimap.size(), 9);
QCOMPARE(multimap2.size(), 12);
multimap = multimap2;
QCOMPARE(multimap.size(), 12);
QCOMPARE(multimap2.size(), 12);
auto size = multimap.size();
for (auto it = multimap.begin(); it != multimap.end();) {
const auto range = multimap.equal_range(it.key());
const auto oldIt = it;
it = range.second;
size -= multimap.remove(oldIt.key());
QCOMPARE(multimap.size(), size);
QCOMPARE(multimap2.size(), 12);
}
QCOMPARE(multimap.size(), 0);
QCOMPARE(multimap2.size(), 12);
}
}
QTEST_APPLESS_MAIN(tst_QMap)
#include "tst_qmap.moc"