From 37c7ef4f4a8478e94eaf0af5b40c279c476fa561 Mon Sep 17 00:00:00 2001 From: Ulf Hermann Date: Mon, 19 Oct 2020 10:07:29 +0200 Subject: [PATCH] QMetaContainer: Consistently coerce types The high-level iterable interfaces should coerce the types of most QVariants passed to the expected ones. To do this, move the type coercion code into qvariant.{h|cpp} so that it is available to the QVariantRef specializations. The exception are variants passed to the find() functions of associative iterables. Here, we should not coerce values we cannot convert to the default-constructed keys. Instead we return end() in such cases. Fixes: QTBUG-87687 Change-Id: I0bd4e5c4e4e270dd3bf36cb3fb115794828077f2 Reviewed-by: Fabian Kosmale --- src/corelib/.prev_CMakeLists.txt | 2 +- src/corelib/CMakeLists.txt | 2 +- src/corelib/kernel/kernel.pri | 1 - src/corelib/kernel/qassociativeiterable.cpp | 34 +++-- src/corelib/kernel/qassociativeiterable.h | 21 +-- src/corelib/kernel/qiterable_p.cpp | 63 --------- src/corelib/kernel/qiterable_p.h | 2 - src/corelib/kernel/qsequentialiterable.cpp | 8 +- src/corelib/kernel/qsequentialiterable.h | 10 +- src/corelib/kernel/qvariant.cpp | 32 +++++ src/corelib/kernel/qvariant.h | 12 ++ .../corelib/kernel/qvariant/tst_qvariant.cpp | 127 ++++++++++++------ 12 files changed, 172 insertions(+), 142 deletions(-) delete mode 100644 src/corelib/kernel/qiterable_p.cpp diff --git a/src/corelib/.prev_CMakeLists.txt b/src/corelib/.prev_CMakeLists.txt index 7a3d45d587..48e6ef5afb 100644 --- a/src/corelib/.prev_CMakeLists.txt +++ b/src/corelib/.prev_CMakeLists.txt @@ -81,7 +81,7 @@ qt_internal_add_module(Core kernel/qelapsedtimer.cpp kernel/qelapsedtimer.h kernel/qeventloop.cpp kernel/qeventloop.h kernel/qfunctions_p.h - kernel/qiterable.cpp kernel/qiterable.h kernel/qiterable_p.cpp kernel/qiterable_p.h + kernel/qiterable.cpp kernel/qiterable.h kernel/qiterable_p.h kernel/qmath.cpp kernel/qmath.h kernel/qmetacontainer.cpp kernel/qmetacontainer.h kernel/qmetaobject.cpp kernel/qmetaobject.h kernel/qmetaobject_p.h diff --git a/src/corelib/CMakeLists.txt b/src/corelib/CMakeLists.txt index e9c934acf4..8a3bc0ab84 100644 --- a/src/corelib/CMakeLists.txt +++ b/src/corelib/CMakeLists.txt @@ -103,7 +103,7 @@ qt_internal_add_module(Core kernel/qelapsedtimer.cpp kernel/qelapsedtimer.h kernel/qeventloop.cpp kernel/qeventloop.h kernel/qfunctions_p.h - kernel/qiterable.cpp kernel/qiterable.h kernel/qiterable_p.cpp kernel/qiterable_p.h + kernel/qiterable.cpp kernel/qiterable.h kernel/qiterable_p.h kernel/qmath.cpp kernel/qmath.h kernel/qmetacontainer.cpp kernel/qmetacontainer.h kernel/qmetaobject.cpp kernel/qmetaobject.h kernel/qmetaobject_p.h diff --git a/src/corelib/kernel/kernel.pri b/src/corelib/kernel/kernel.pri index 518caa4494..0b6c8cbc76 100644 --- a/src/corelib/kernel/kernel.pri +++ b/src/corelib/kernel/kernel.pri @@ -75,7 +75,6 @@ SOURCES += \ kernel/qvariant.cpp \ kernel/qcoreglobaldata.cpp \ kernel/qiterable.cpp \ - kernel/qiterable_p.cpp \ kernel/qsequentialiterable.cpp \ kernel/qassociativeiterable.cpp \ kernel/qsharedmemory.cpp \ diff --git a/src/corelib/kernel/qassociativeiterable.cpp b/src/corelib/kernel/qassociativeiterable.cpp index 780ae35a42..5e30ae291e 100644 --- a/src/corelib/kernel/qassociativeiterable.cpp +++ b/src/corelib/kernel/qassociativeiterable.cpp @@ -187,40 +187,46 @@ QVariantConstPointer QAssociativeConstIterator::operator->() const /*! Retrieves a const_iterator pointing to the element at the given \a key, or - the end of the container if that key does not exist. + the end of the container if that key does not exist. If the \a key isn't + convertible to the expected type, the end of the container is returned. */ QAssociativeIterable::const_iterator QAssociativeIterable::find(const QVariant &key) const { const QMetaAssociation meta = metaContainer(); - QVariant converted = key; - const void *keyData = QIterablePrivate::coerceType(converted, meta.keyMetaType()); - return const_iterator(QConstIterator( - this, meta.createConstIteratorAtKey(constIterable(), keyData))); + QtPrivate::QVariantTypeCoercer coercer; + if (const void *keyData = coercer.convert(key, meta.keyMetaType())) { + return const_iterator(QConstIterator(this, meta.createConstIteratorAtKey( + constIterable(), keyData))); + } + return constEnd(); } /*! Retrieves an iterator pointing to the element at the given \a key, or - the end of the container if that key does not exist. + the end of the container if that key does not exist. If the \a key isn't + convertible to the expected type, the end of the container is returned. */ QAssociativeIterable::iterator QAssociativeIterable::mutableFind(const QVariant &key) { const QMetaAssociation meta = metaContainer(); - QVariant converted = key; - const void *keyData = QIterablePrivate::coerceType(converted, meta.keyMetaType()); - return iterator(QIterator(this, meta.createIteratorAtKey(mutableIterable(), keyData))); + QtPrivate::QVariantTypeCoercer coercer; + if (const void *keyData = coercer.convert(key, meta.keyMetaType())) + return iterator(QIterator(this, meta.createIteratorAtKey(mutableIterable(), keyData))); + return mutableEnd(); } /*! - Retrieves the mapped value at the given \a key, or an invalid QVariant - if the key does not exist. + Retrieves the mapped value at the given \a key, or a default-constructed + QVariant of the mapped type, if the key does not exist. If the \a key is not + convertible to the key type, the mapped value associated with the + default-constructed key is returned. */ QVariant QAssociativeIterable::value(const QVariant &key) const { const QMetaAssociation meta = metaContainer(); - QVariant converted = key; - const void *keyData = QIterablePrivate::coerceType(converted, meta.keyMetaType()); + QtPrivate::QVariantTypeCoercer coercer; QVariant result(QMetaType(meta.mappedMetaType())); - meta.mappedAtKey(constIterable(), keyData, result.data()); + meta.mappedAtKey(constIterable(), coercer.coerce(key, meta.keyMetaType()), result.data()); return result; } diff --git a/src/corelib/kernel/qassociativeiterable.h b/src/corelib/kernel/qassociativeiterable.h index 8412dc86fd..ba20af0327 100644 --- a/src/corelib/kernel/qassociativeiterable.h +++ b/src/corelib/kernel/qassociativeiterable.h @@ -163,14 +163,15 @@ inline QVariantRef::operator QVariant() const { if (m_pointer == nullptr) return QVariant(); + const auto metaAssociation = m_pointer->metaContainer(); - QMetaType metaType(metaAssociation.mappedMetaType()); + const QMetaType metaType(metaAssociation.mappedMetaType()); if (!metaType.isValid()) return m_pointer->key(); QVariant v(metaType); - void *dataPtr = metaType == QMetaType::fromType() ? &v : v.data(); - metaAssociation.mappedAtIterator(m_pointer->constIterator(), dataPtr); + metaAssociation.mappedAtIterator(m_pointer->constIterator(), + metaType == QMetaType::fromType() ? &v : v.data()); return v; } @@ -180,11 +181,15 @@ inline QVariantRef &QVariantRef::ope { if (m_pointer == nullptr) return *this; - const QMetaType metaType(m_pointer->metaContainer().mappedMetaType()); - const void *dataPtr = metaType == QMetaType::fromType() - ? &value - : value.constData(); - m_pointer->metaContainer().setMappedAtIterator(m_pointer->constIterator(), dataPtr); + + const auto metaAssociation = m_pointer->metaContainer(); + const QMetaType metaType(metaAssociation.mappedMetaType()); + if (metaType.isValid()) { + QtPrivate::QVariantTypeCoercer coercer; + metaAssociation.setMappedAtIterator(m_pointer->constIterator(), + coercer.coerce(value, metaType)); + } + return *this; } diff --git a/src/corelib/kernel/qiterable_p.cpp b/src/corelib/kernel/qiterable_p.cpp deleted file mode 100644 index ea569cebd5..0000000000 --- a/src/corelib/kernel/qiterable_p.cpp +++ /dev/null @@ -1,63 +0,0 @@ -/**************************************************************************** -** -** Copyright (C) 2020 The Qt Company Ltd. -** Contact: https://www.qt.io/licensing/ -** -** This file is part of the QtCore module of the Qt Toolkit. -** -** $QT_BEGIN_LICENSE:LGPL$ -** Commercial License Usage -** Licensees holding valid commercial Qt licenses may use this file in -** accordance with the commercial license agreement provided with the -** Software or, alternatively, in accordance with the terms contained in -** a written agreement between you and The Qt Company. For licensing terms -** and conditions see https://www.qt.io/terms-conditions. For further -** information use the contact form at https://www.qt.io/contact-us. -** -** GNU Lesser General Public License Usage -** Alternatively, this file may be used under the terms of the GNU Lesser -** General Public License version 3 as published by the Free Software -** Foundation and appearing in the file LICENSE.LGPL3 included in the -** packaging of this file. Please review the following information to -** ensure the GNU Lesser General Public License version 3 requirements -** will be met: https://www.gnu.org/licenses/lgpl-3.0.html. -** -** GNU General Public License Usage -** Alternatively, this file may be used under the terms of the GNU -** General Public License version 2.0 or (at your option) the GNU General -** Public license version 3 or any later version approved by the KDE Free -** Qt Foundation. The licenses are as published by the Free Software -** Foundation and appearing in the file LICENSE.GPL2 and LICENSE.GPL3 -** included in the packaging of this file. Please review the following -** information to ensure the GNU General Public License requirements will -** be met: https://www.gnu.org/licenses/gpl-2.0.html and -** https://www.gnu.org/licenses/gpl-3.0.html. -** -** $QT_END_LICENSE$ -** -****************************************************************************/ - -#include - -QT_BEGIN_NAMESPACE - -namespace QIterablePrivate { - -const void *coerceType(QVariant &value, const QMetaType &type) -{ - if (type == QMetaType::fromType()) { - return &value; - } else if (type == value.metaType()) { - return value.constData(); - } else if (value.canConvert(type)) { - value.convert(type); - return value.constData(); - } - - value = QVariant(type); - return value.constData(); -} - -} // namespace QIterablePrivate - -QT_END_NAMESPACE diff --git a/src/corelib/kernel/qiterable_p.h b/src/corelib/kernel/qiterable_p.h index 33b190a59a..979ef49b18 100644 --- a/src/corelib/kernel/qiterable_p.h +++ b/src/corelib/kernel/qiterable_p.h @@ -58,8 +58,6 @@ QT_BEGIN_NAMESPACE namespace QIterablePrivate { -const void *coerceType(QVariant &value, const QMetaType &type); - template static QVariant retrieveElement(const QMetaType &type, Callback callback) { diff --git a/src/corelib/kernel/qsequentialiterable.cpp b/src/corelib/kernel/qsequentialiterable.cpp index 6ce41a1edf..054d97d90f 100644 --- a/src/corelib/kernel/qsequentialiterable.cpp +++ b/src/corelib/kernel/qsequentialiterable.cpp @@ -106,8 +106,8 @@ QT_BEGIN_NAMESPACE */ void QSequentialIterable::addValue(const QVariant &value, Position position) { - QVariant converted = value; - const void *valuePtr = QIterablePrivate::coerceType(converted, metaContainer().valueMetaType()); + QtPrivate::QVariantTypeCoercer coercer; + const void *valuePtr = coercer.coerce(value, metaContainer().valueMetaType()); switch (position) { case AtBegin: @@ -181,8 +181,8 @@ QVariant QSequentialIterable::at(qsizetype idx) const */ void QSequentialIterable::set(qsizetype idx, const QVariant &value) { - QVariant converted = value; - const void *dataPtr = QIterablePrivate::coerceType(converted, metaContainer().valueMetaType()); + QtPrivate::QVariantTypeCoercer coercer; + const void *dataPtr = coercer.coerce(value, metaContainer().valueMetaType()); const QMetaSequence meta = metaContainer(); if (meta.canSetValueAtIndex()) { diff --git a/src/corelib/kernel/qsequentialiterable.h b/src/corelib/kernel/qsequentialiterable.h index 5851439316..a5a22b55df 100644 --- a/src/corelib/kernel/qsequentialiterable.h +++ b/src/corelib/kernel/qsequentialiterable.h @@ -172,11 +172,11 @@ inline QVariantRef &QVariantRef::opera { if (m_pointer == nullptr) return *this; - const QMetaType metaType(m_pointer->metaContainer().valueMetaType()); - const void *dataPtr = metaType == QMetaType::fromType() - ? &value - : value.constData(); - m_pointer->metaContainer().setValueAtIterator(m_pointer->constIterator(), dataPtr); + + QtPrivate::QVariantTypeCoercer coercer; + m_pointer->metaContainer().setValueAtIterator( + m_pointer->constIterator(), + coercer.coerce(value, m_pointer->metaContainer().valueMetaType())); return *this; } diff --git a/src/corelib/kernel/qvariant.cpp b/src/corelib/kernel/qvariant.cpp index 09c3eee6ea..849ed6e6ef 100644 --- a/src/corelib/kernel/qvariant.cpp +++ b/src/corelib/kernel/qvariant.cpp @@ -2728,6 +2728,38 @@ QDebug operator<<(QDebug dbg, const QVariant::Type p) \internal */ +/*! + \internal + */ +const void *QtPrivate::QVariantTypeCoercer::convert(const QVariant &value, const QMetaType &type) +{ + if (type == QMetaType::fromType()) + return &value; + + if (type == value.metaType()) + return value.constData(); + + if (value.canConvert(type)) { + converted = value; + if (converted.convert(type)) + return converted.constData(); + } + + return nullptr; +} + +/*! + \internal + */ +const void *QtPrivate::QVariantTypeCoercer::coerce(const QVariant &value, const QMetaType &type) +{ + if (const void *result = convert(value, type)) + return result; + + converted = QVariant(type); + return converted.constData(); +} + /*! \class QVariantRef \since 6.0 diff --git a/src/corelib/kernel/qvariant.h b/src/corelib/kernel/qvariant.h index 40d3d5c3b1..aa74aaa18f 100644 --- a/src/corelib/kernel/qvariant.h +++ b/src/corelib/kernel/qvariant.h @@ -605,6 +605,18 @@ template<> inline QVariant qvariant_cast(const QVariant &v) Q_CORE_EXPORT QDebug operator<<(QDebug, const QVariant::Type); #endif +namespace QtPrivate { +class Q_CORE_EXPORT QVariantTypeCoercer +{ +public: + const void *convert(const QVariant &value, const QMetaType &type); + const void *coerce(const QVariant &value, const QMetaType &type); + +private: + QVariant converted; +}; +} + template class QVariantRef { diff --git a/tests/auto/corelib/kernel/qvariant/tst_qvariant.cpp b/tests/auto/corelib/kernel/qvariant/tst_qvariant.cpp index 8df3b4055b..4ff404e12a 100644 --- a/tests/auto/corelib/kernel/qvariant/tst_qvariant.cpp +++ b/tests/auto/corelib/kernel/qvariant/tst_qvariant.cpp @@ -57,6 +57,8 @@ #include #include "tst_qvariant_common.h" +#include + class CustomNonQObject; class tst_QVariant : public QObject @@ -4150,11 +4152,6 @@ struct KeyGetter > }; -// We have no built-in defines to check the stdlib features. -// #define TEST_UNORDERED_MAP - -#ifdef TEST_UNORDERED_MAP -#include typedef std::unordered_map StdUnorderedMap_int_bool; Q_DECLARE_ASSOCIATIVE_CONTAINER_METATYPE(std::unordered_map) @@ -4173,7 +4170,6 @@ struct KeyGetter > return it->second; } }; -#endif template void sortIterable(QSequentialIterable *iterable) @@ -4290,6 +4286,83 @@ void testSequentialIteration() QCOMPARE(listIter.at(2), second); QCOMPARE(listIter.at(3), second); QCOMPARE(listIter.at(4), third); + + auto i = listIter.mutableBegin(); + QVERIFY(i != listIter.mutableEnd()); + + *i = QStringLiteral("17"); + if (listIter.metaContainer().valueMetaType() == QMetaType::fromType()) + QCOMPARE(listIter.at(0).toInt(), 17); + else if (listIter.metaContainer().valueMetaType() == QMetaType::fromType()) + QCOMPARE(listIter.at(0).toBool(), false); + + *i = QStringLiteral("true"); + if (listIter.metaContainer().valueMetaType() == QMetaType::fromType()) + QCOMPARE(listIter.at(0).toInt(), 0); + else if (listIter.metaContainer().valueMetaType() == QMetaType::fromType()) + QCOMPARE(listIter.at(0).toBool(), true); +} + +template +void testAssociativeIteration() +{ + using Key = typename Container::key_type; + using Mapped = typename Container::mapped_type; + + int numSeen = 0; + Container mapping; + mapping[5] = true; + mapping[15] = false; + + QVariant mappingVariant = QVariant::fromValue(mapping); + QVariantMap varMap = mappingVariant.value(); + QVariantMap varHash = mappingVariant.value(); + QAssociativeIterable mappingIter = mappingVariant.view(); + + typename Container::const_iterator containerIter = mapping.begin(); + const typename Container::const_iterator containerEnd = mapping.end(); + for ( ;containerIter != containerEnd; ++containerIter, ++numSeen) + { + Mapped expected = KeyGetter::value(containerIter); + Key key = KeyGetter::get(containerIter); + Mapped actual = qvariant_cast(mappingIter.value(key)); + QCOMPARE(qvariant_cast(varMap.value(QString::number(key))), expected); + QCOMPARE(qvariant_cast(varHash.value(QString::number(key))), expected); + QCOMPARE(actual, expected); + const QAssociativeIterable::const_iterator it = mappingIter.find(key); + QVERIFY(it != mappingIter.end()); + QCOMPARE(it.value().value(), expected); + } + QCOMPARE(numSeen, (int)std::distance(mapping.begin(), mapping.end())); + QCOMPARE(containerIter, containerEnd); + QVERIFY(mappingIter.find(10) == mappingIter.end()); + + auto i = mappingIter.mutableFind(QStringLiteral("nonono")); + QCOMPARE(i, mappingIter.mutableEnd()); + i = mappingIter.mutableFind(QStringLiteral("5")); + QVERIFY(i != mappingIter.mutableEnd()); + + *i = QStringLiteral("17"); + + if (mappingIter.metaContainer().mappedMetaType() == QMetaType::fromType()) + QCOMPARE(mappingIter.value(5).toInt(), 17); + else if (mappingIter.metaContainer().mappedMetaType() == QMetaType::fromType()) + QCOMPARE(mappingIter.value(5).toBool(), true); + + *i = QStringLiteral("true"); + if (mappingIter.metaContainer().mappedMetaType() == QMetaType::fromType()) + QCOMPARE(mappingIter.value(5).toInt(), 0); + else if (mappingIter.metaContainer().mappedMetaType() == QMetaType::fromType()) + QCOMPARE(mappingIter.value(5).toBool(), true); + + // Test that find() does not coerce + auto container = Container(); + container[0] = true; + + QVariant containerVariant = QVariant::fromValue(container); + QAssociativeIterable iter = containerVariant.value(); + auto f = iter.constFind(QStringLiteral("anything")); + QCOMPARE(f, iter.constEnd()); } void tst_QVariant::iterateContainerElements() @@ -4341,43 +4414,11 @@ void tst_QVariant::iterateContainerElements() QCOMPARE(ints, intsCopy); } -#define TEST_ASSOCIATIVE_ITERATION(CONTAINER, KEY_TYPE, MAPPED_TYPE) \ - { \ - int numSeen = 0; \ - CONTAINER mapping; \ - mapping[5] = true; \ - mapping[15] = false; \ - \ - QVariant mappingVariant = QVariant::fromValue(mapping); \ - QVariantMap varMap = mappingVariant.value(); \ - QVariantMap varHash = mappingVariant.value(); \ - QAssociativeIterable mappingIter = mappingVariant.value(); \ - \ - CONTAINER::const_iterator containerIter = mapping.begin(); \ - const CONTAINER::const_iterator containerEnd = mapping.end(); \ - for ( ; containerIter != containerEnd; ++containerIter, ++numSeen) \ - { \ - MAPPED_TYPE expected = KeyGetter >::value(containerIter); \ - KEY_TYPE key = KeyGetter >::get(containerIter); \ - MAPPED_TYPE actual = mappingIter.value(key).value(); \ - QCOMPARE(varMap.value(QString::number(key)).value(), expected); \ - QCOMPARE(varHash.value(QString::number(key)).value(), expected); \ - QCOMPARE(actual, expected); \ - const QAssociativeIterable::const_iterator it = mappingIter.find(key); \ - QVERIFY(it != mappingIter.end()); \ - QCOMPARE(it.value().value(), expected); \ - } \ - QCOMPARE(numSeen, (int)std::distance(mapping.begin(), mapping.end())); \ - QCOMPARE(containerIter, containerEnd); \ - QVERIFY(mappingIter.find(10) == mappingIter.end()); \ - } - - TEST_ASSOCIATIVE_ITERATION(QHash, int, bool) - TEST_ASSOCIATIVE_ITERATION(QMap, int, bool) - TEST_ASSOCIATIVE_ITERATION(std::map, int, bool) -#ifdef TEST_UNORDERED_MAP - TEST_ASSOCIATIVE_ITERATION(std::unordered_map, int, bool) -#endif + testAssociativeIteration>(); + testAssociativeIteration>(); + testAssociativeIteration>(); + testAssociativeIteration>(); + testAssociativeIteration>(); { QMap mapping;