From 285a2a75b4342e73d3165378d7af6c48d2f06492 Mon Sep 17 00:00:00 2001 From: Ivan Solovev Date: Thu, 7 Mar 2024 16:24:00 +0100 Subject: [PATCH] QVariant: use comparison helper macros The relational operators were removed in 8652c79df0a47264a2d525424484e15744e2462b with the argument that they do not have a total order. Back than it was a valid argument, because Qt did not support any of the C++20 ordering types. Now Qt has its own implementation for all three ordering types, so we could technically add relational operators and claim that QVariant provides partial ordering. However, that could potentially lead to many bugs and/or unexpected results, if people start using QVariant as a key in std::map/QMap containers. We do not want that to happen, so we only use the helper macros to implement (in)equality operators. This commit also extends the unit tests, providing more extensive testing of (in)equality operators and the pre-existing QVariant::compare() method. Fixes: QTBUG-113234 Change-Id: I783f3b5df552da782627f4ed0a5bb1b577753a23 Reviewed-by: Thiago Macieira --- src/corelib/kernel/qvariant.cpp | 9 +- src/corelib/kernel/qvariant.h | 7 +- .../corelib/kernel/qvariant/CMakeLists.txt | 1 + .../corelib/kernel/qvariant/tst_qvariant.cpp | 107 ++++++++++++------ 4 files changed, 81 insertions(+), 43 deletions(-) diff --git a/src/corelib/kernel/qvariant.cpp b/src/corelib/kernel/qvariant.cpp index 270d2642e9..92a44c462b 100644 --- a/src/corelib/kernel/qvariant.cpp +++ b/src/corelib/kernel/qvariant.cpp @@ -324,6 +324,7 @@ static QVariant::Private clonePrivate(const QVariant::Private &other) \ingroup objectmodel \ingroup shared + \compares equality Because C++ forbids unions from including types that have non-default constructors or destructors, most interesting Qt @@ -2152,9 +2153,9 @@ bool QVariant::view(int type, void *ptr) } /*! - \fn bool QVariant::operator==(const QVariant &v1, const QVariant &v2) + \fn bool QVariant::operator==(const QVariant &lhs, const QVariant &rhs) - Returns \c true if \a v1 and \a v2 are equal; otherwise returns \c false. + Returns \c true if \a lhs and \a rhs are equal; otherwise returns \c false. QVariant uses the equality operator of the type() contained to check for equality. @@ -2178,9 +2179,9 @@ bool QVariant::view(int type, void *ptr) */ /*! - \fn bool QVariant::operator!=(const QVariant &v1, const QVariant &v2) + \fn bool QVariant::operator!=(const QVariant &lhs, const QVariant &rhs) - Returns \c false if \a v1 and \a v2 are equal; otherwise returns \c true. + Returns \c false if \a lhs and \a rhs are equal; otherwise returns \c true. QVariant uses the equality operator of the type() contained to check for equality. diff --git a/src/corelib/kernel/qvariant.h b/src/corelib/kernel/qvariant.h index 6d51d940c5..9f0edf4233 100644 --- a/src/corelib/kernel/qvariant.h +++ b/src/corelib/kernel/qvariant.h @@ -5,6 +5,7 @@ #define QVARIANT_H #include +#include #include #include #ifndef QT_NO_DEBUG_STREAM @@ -614,10 +615,10 @@ private: return std::visit(visitor, std::forward(v)); } - friend inline bool operator==(const QVariant &a, const QVariant &b) + friend bool comparesEqual(const QVariant &a, const QVariant &b) { return a.equals(b); } - friend inline bool operator!=(const QVariant &a, const QVariant &b) - { return !a.equals(b); } + Q_DECLARE_EQUALITY_COMPARABLE(QVariant) + #ifndef QT_NO_DEBUG_STREAM template friend auto operator<<(const QDebug &debug, const T &variant) -> std::enable_if_t, QDebug> { diff --git a/tests/auto/corelib/kernel/qvariant/CMakeLists.txt b/tests/auto/corelib/kernel/qvariant/CMakeLists.txt index 4a1eb18d4f..eae9c0d30e 100644 --- a/tests/auto/corelib/kernel/qvariant/CMakeLists.txt +++ b/tests/auto/corelib/kernel/qvariant/CMakeLists.txt @@ -25,6 +25,7 @@ qt_internal_add_test(tst_qvariant LIBRARIES Qt::CorePrivate Qt::Gui + Qt::TestPrivate TESTDATA ${qvariant_resource_files} BUILTIN_TESTDATA ) diff --git a/tests/auto/corelib/kernel/qvariant/tst_qvariant.cpp b/tests/auto/corelib/kernel/qvariant/tst_qvariant.cpp index 84456a57b2..75907ec062 100644 --- a/tests/auto/corelib/kernel/qvariant/tst_qvariant.cpp +++ b/tests/auto/corelib/kernel/qvariant/tst_qvariant.cpp @@ -76,6 +76,7 @@ CHECK_GET(MyVariant, const &&); #include #include +#include #include #include #include "tst_qvariant_common.h" @@ -290,6 +291,7 @@ private slots: void variantHash(); void convertToQUint8() const; + void compareCompiles() const; void compareNumerics_data() const; void compareNumerics() const; void comparePointers() const; @@ -305,6 +307,7 @@ private slots: void loadBrokenUserType(); void invalidDate() const; + void compareCustomTypes_data() const; void compareCustomTypes() const; void timeToDateTime() const; void copyingUserTypes() const; @@ -2007,7 +2010,7 @@ void tst_QVariant::operator_eq_eq() QFETCH( QVariant, left ); QFETCH( QVariant, right ); QFETCH( bool, equal ); - QCOMPARE( left == right, equal ); + QT_TEST_EQUALITY_OPS(left, right, equal); } #if QT_DEPRECATED_SINCE(6, 0) @@ -2980,6 +2983,11 @@ void tst_QVariant::convertToQUint8() const } } +void tst_QVariant::compareCompiles() const +{ + QTestPrivate::testEqualityOperatorsCompile(); +} + void tst_QVariant::compareNumerics_data() const { QTest::addColumn("v1"); @@ -3225,25 +3233,38 @@ void tst_QVariant::compareNumerics() const QFETCH(QPartialOrdering, result); QCOMPARE(QVariant::compare(v1, v2), result); - QEXPECT_FAIL("invalid-invalid", "needs fixing", Continue); - if (result == QPartialOrdering::Equivalent) - QCOMPARE_EQ(v1, v2); - else - QCOMPARE_NE(v1, v2); + QEXPECT_FAIL("invalid-invalid", "needs fixing", Abort); + QT_TEST_EQUALITY_OPS(v1, v2, is_eq(result)); } void tst_QVariant::comparePointers() const { - class MyClass + class NonQObjectClass {}; + const std::array arr{ NonQObjectClass{}, NonQObjectClass{} }; + + const QVariant nonObjV1 = QVariant::fromValue(&arr[0]); + const QVariant nonObjV2 = QVariant::fromValue(&arr[1]); + + Qt::partial_ordering expectedOrdering = Qt::partial_ordering::equivalent; + QCOMPARE(QVariant::compare(nonObjV1, nonObjV1), expectedOrdering); + QT_TEST_EQUALITY_OPS(nonObjV1, nonObjV1, is_eq(expectedOrdering)); + + expectedOrdering = Qt::partial_ordering::less; + QCOMPARE(QVariant::compare(nonObjV1, nonObjV2), expectedOrdering); + QT_TEST_EQUALITY_OPS(nonObjV1, nonObjV2, is_eq(expectedOrdering)); + + class QObjectClass : public QObject { + public: + QObjectClass(QObject *parent = nullptr) : QObject(parent) {} }; + const QObjectClass c1; + const QObjectClass c2; - MyClass myClass; - - QVariant v = QVariant::fromValue(&myClass); - QVariant v2 = QVariant::fromValue(&myClass); - - QCOMPARE(v, v2); + const QVariant objV1 = QVariant::fromValue(&c1); + const QVariant objV2 = QVariant::fromValue(&c2); + QT_TEST_EQUALITY_OPS(objV1, objV1, true); + QT_TEST_EQUALITY_OPS(objV1, objV2, false); } struct Data {}; @@ -3442,35 +3463,49 @@ Q_DECLARE_METATYPE(WontCompare); struct WillCompare { int x; + + friend bool operator==(const WillCompare &a, const WillCompare &b) + { return a.x == b.x; } + friend bool operator<(const WillCompare &a, const WillCompare &b) + { return a.x < b.x; } }; -bool operator==(const WillCompare &a, const WillCompare &b) { return a.x == b.x; } Q_DECLARE_METATYPE(WillCompare); +void tst_QVariant::compareCustomTypes_data() const +{ + QTest::addColumn("v1"); + QTest::addColumn("v2"); + QTest::addColumn("expectedOrdering"); + + QTest::newRow("same_uncomparable") + << QVariant::fromValue(WontCompare{0}) + << QVariant::fromValue(WontCompare{0}) + << Qt::partial_ordering::unordered; + + QTest::newRow("same_comparable") + << QVariant::fromValue(WillCompare{0}) + << QVariant::fromValue(WillCompare{0}) + << Qt::partial_ordering::equivalent; + + QTest::newRow("different_comparable") + << QVariant::fromValue(WillCompare{1}) + << QVariant::fromValue(WillCompare{0}) + << Qt::partial_ordering::greater; + + QTest::newRow("qdatetime_vs_comparable") + << QVariant::fromValue(QDateTime::currentDateTimeUtc()) + << QVariant::fromValue(WillCompare{0}) + << Qt::partial_ordering::unordered; +} + void tst_QVariant::compareCustomTypes() const { - { - WontCompare f1{0}; - const QVariant variant1(QVariant::fromValue(f1)); + QFETCH(const QVariant, v1); + QFETCH(const QVariant, v2); + QFETCH(const Qt::partial_ordering, expectedOrdering); - WontCompare f2{1}; - const QVariant variant2(QVariant::fromValue(f2)); - - /* No comparison operator exists. */ - QVERIFY(variant1 != variant2); - QVERIFY(variant1 != variant1); - QVERIFY(variant2 != variant2); - } - { - WillCompare f1{0}; - const QVariant variant1(QVariant::fromValue(f1)); - - WillCompare f2 {1}; - const QVariant variant2(QVariant::fromValue(f2)); - - QVERIFY(variant1 != variant2); - QCOMPARE(variant1, variant1); - QCOMPARE(variant2, variant2); - } + QCOMPARE(QVariant::compare(v1, v2), expectedOrdering); + QT_TEST_EQUALITY_OPS(v1, v2, is_eq(expectedOrdering)); } void tst_QVariant::timeToDateTime() const {