From e99417106f4a2a7ef75cdf3317d950c30ad78ebe Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Sat, 27 Nov 2021 10:57:24 -0800 Subject: [PATCH] QJsonObject::erase: erase unnecessary and wrong code Commit 35adb74ddd915831789f0175423660f8e898942e ("Reimplement JSON support on top of Cbor") accidentally forgot to multiply by 2 the index stored in the QJsonObject::iterator. The same mistake was propagated when QJsonObject::iterator was converted to QJsonValueRef. This had no ill effects because the o->elements container would always contain more elements, but it meant the check was ineffective and meant nothing. So instead of doing nothing when the iterator does not point to this container, simply assume it does. Bad things will happen if you try to erase an iterator that points to another container, but that's true for almost all container/iterator mechanisms. Drive-by modernization of some of the surrounding lines. Change-Id: I89446ea06b5742efb194fffd16bb7c322c2fc4f2 Reviewed-by: Ulf Hermann Reviewed-by: Edward Welbourne --- src/corelib/serialization/qjsonobject.cpp | 9 ++- .../corelib/serialization/json/tst_qtjson.cpp | 71 ++++++++++++++++--- 2 files changed, 66 insertions(+), 14 deletions(-) diff --git a/src/corelib/serialization/qjsonobject.cpp b/src/corelib/serialization/qjsonobject.cpp index 502d64d854..014224c4db 100644 --- a/src/corelib/serialization/qjsonobject.cpp +++ b/src/corelib/serialization/qjsonobject.cpp @@ -707,13 +707,12 @@ bool QJsonObject::operator!=(const QJsonObject &other) const */ QJsonObject::iterator QJsonObject::erase(QJsonObject::iterator it) { - if (it.item.o != this || qsizetype(it.item.index) >= o->elements.length()) - return {this, o->elements.length()}; - removeAt(it.item.index); - // iterator hasn't changed - return it; + // index hasn't changed; the container pointer shouldn't have changed + // because we shouldn't have detached (detaching happens on obtaining a + // non-const iterator). But just in case we did, reload the pointer. + return { this, qsizetype(it.item.index) }; } #if QT_STRINGVIEW_LEVEL < 2 diff --git a/tests/auto/corelib/serialization/json/tst_qtjson.cpp b/tests/auto/corelib/serialization/json/tst_qtjson.cpp index 5a2ed0a85d..1b6148429f 100644 --- a/tests/auto/corelib/serialization/json/tst_qtjson.cpp +++ b/tests/auto/corelib/serialization/json/tst_qtjson.cpp @@ -1040,14 +1040,37 @@ void tst_QtJson::testObjectIteration() QCOMPARE(object, object2); QJsonValue val = *object2.begin(); - object2.erase(object2.begin()); + auto next = object2.erase(object2.begin()); QCOMPARE(object.size(), 10); QCOMPARE(object2.size(), 9); + QVERIFY(next == object2.begin()); - for (QJsonObject::const_iterator it = object2.constBegin(); it != object2.constEnd(); ++it) { + double d = 1; // we erased the first item + for (auto it = object2.constBegin(); it != object2.constEnd(); ++it, d += 1) { QJsonValue value = it.value(); QVERIFY(it.value() != val); - QCOMPARE((double)it.key().toInt(), value.toDouble()); + QCOMPARE(it.value(), d); + QCOMPARE(it.value().toDouble(), d); + QCOMPARE(it.key().toInt(), value.toDouble()); + } + } + + { + QJsonObject object2 = object; + QCOMPARE(object, object2); + + QJsonValue val = *(object2.end() - 1); + auto next = object2.erase(object2.end() - 1); + QCOMPARE(object.size(), 10); + QCOMPARE(object2.size(), 9); + QVERIFY(next == object2.end()); + double d = 0; + for (auto it = object2.constBegin(); it != object2.constEnd(); ++it, d += 1) { + QJsonValue value = it.value(); + QVERIFY(it.value() != val); + QCOMPARE(it.value(), d); + QCOMPARE(it.value().toDouble(), d); + QCOMPARE(it.key().toInt(), value.toDouble()); } } @@ -1057,14 +1080,20 @@ void tst_QtJson::testObjectIteration() QJsonObject::iterator it = object2.find(QString::number(5)); QJsonValue val = *it; - object2.erase(it); + auto next = object2.erase(it); QCOMPARE(object.size(), 10); QCOMPARE(object2.size(), 9); + QCOMPARE(*next, 6); - for (QJsonObject::const_iterator it = object2.constBegin(); it != object2.constEnd(); ++it) { + int i = 0; + for (auto it = object2.constBegin(); it != object2.constEnd(); ++it, ++i) { + if (i == 5) + ++i; QJsonValue value = it.value(); QVERIFY(it.value() != val); - QCOMPARE((double)it.key().toInt(), value.toDouble()); + QCOMPARE(it.value(), i); + QCOMPARE(it.value().toInt(), i); + QCOMPARE(it.key().toInt(), value.toDouble()); } } @@ -1120,14 +1149,38 @@ void tst_QtJson::testArrayIteration() QCOMPARE(array, array2); QJsonValue val = *array2.begin(); - array2.erase(array2.begin()); + auto next = array2.erase(array2.begin()); QCOMPARE(array.size(), 10); QCOMPARE(array2.size(), 9); + QVERIFY(next == array2.begin()); i = 1; - for (QJsonArray::const_iterator it = array2.constBegin(); it != array2.constEnd(); ++it, ++i) { + for (auto it = array2.constBegin(); it != array2.constEnd(); ++it, ++i) { QJsonValue value = (*it); - QCOMPARE((double)i, value.toDouble()); + QCOMPARE(value.toInt(), i); + QCOMPARE(value.toDouble(), i); + QCOMPARE(it->toInt(), i); + QCOMPARE(it->toDouble(), i); + } + } + + { + QJsonArray array2 = array; + QCOMPARE(array, array2); + + QJsonValue val = array2.last(); + auto next = array2.erase(array2.end() - 1); + QCOMPARE(array.size(), 10); + QCOMPARE(array2.size(), 9); + QVERIFY(next == array2.end()); + + i = 0; + for (auto it = array2.constBegin(); it != array2.constEnd(); ++it, ++i) { + QJsonValue value = (*it); + QCOMPARE(value.toInt(), i); + QCOMPARE(value.toDouble(), i); + QCOMPARE(it->toInt(), i); + QCOMPARE(it->toDouble(), i); } }