Use the new support for comparisons in QMetaType for QVariant

Remove the compare method in the QVariant::Handler struct. Rely
on the generic support provided by QMetaType instead.

[ChangeLog][Important Behavior Changes][QVariant] QVariant will now use builtin support in
QMetaType to compare its content. This implies a behavioral change
for some graphical types like QPixmap, QImage and QIcon that will
never compare equal in Qt 6 (as they do not have a comparison
operator).

Change-Id: I30a6e7116c89124d11ed9052537cecc23f78116e
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>

src/corelib/kernel/qvariant.cpp

@@ -123,18 +123,6 @@ static bool isNull(const QVariant::Private *d)
     return QMetaTypeSwitcher::switcher<bool>(isNull, d->type().id());
 }
 
-/*!
-  \internal
-
-  Compares \a a to \a b. The caller guarantees that \a a and \a b
-  are of the same type.
- */
-static bool compare(const QVariant::Private *a, const QVariant::Private *b)
-{
-    QVariantComparator<CoreTypesFilter> comparator(a, b);
-    return QMetaTypeSwitcher::switcher<bool>(comparator, a->type().id());
-}
-
 /*!
   \internal
  */
@@ -1390,7 +1378,6 @@ static void streamDebug(QDebug dbg, const QVariant &v)
 
 const QVariant::Handler qt_kernel_variant_handler = {
     isNull,
-    compare,
     convert,
 #if !defined(QT_NO_DEBUG_STREAM)
     streamDebug
@@ -1400,14 +1387,12 @@ const QVariant::Handler qt_kernel_variant_handler = {
 };
 
 static bool dummyIsNull(const QVariant::Private *d) { Q_ASSERT_X(false, "QVariant::isNull", "Trying to call isNull on an unknown type"); return d->is_null; }
-static bool dummyCompare(const QVariant::Private *, const QVariant::Private *) { Q_ASSERT_X(false, "QVariant", "Trying to compare an unknown types"); return false; }
 static bool dummyConvert(const QVariant::Private *, int, void *, bool *) { Q_ASSERT_X(false, "QVariant", "Trying to convert an unknown type"); return false; }
 #if !defined(QT_NO_DEBUG_STREAM)
 static void dummyStreamDebug(QDebug, const QVariant &) { Q_ASSERT_X(false, "QVariant", "Trying to convert an unknown type"); }
 #endif
 const QVariant::Handler qt_dummy_variant_handler = {
     dummyIsNull,
-    dummyCompare,
     dummyConvert,
 #if !defined(QT_NO_DEBUG_STREAM)
     dummyStreamDebug
@@ -1468,17 +1453,6 @@ static bool customIsNull(const QVariant::Private *d)
     return false;
 }
 
-static bool customCompare(const QVariant::Private *a, const QVariant::Private *b)
-{
-    const void *a_ptr = a->is_shared ? a->data.shared->ptr : &(a->data);
-    const void *b_ptr = b->is_shared ? b->data.shared->ptr : &(b->data);
-
-    if (a->is_null && b->is_null)
-        return true;
-
-    return !memcmp(a_ptr, b_ptr, a->type().sizeOf());
-}
-
 static bool customConvert(const QVariant::Private *d, int t, void *result, bool *ok)
 {
     if (d->type().id() >= QMetaType::User || t >= QMetaType::User) {
@@ -1506,7 +1480,6 @@ static void customStreamDebug(QDebug dbg, const QVariant &variant) {
 
 const QVariant::Handler qt_custom_variant_handler = {
     customIsNull,
-    customCompare,
     customConvert,
 #if !defined(QT_NO_DEBUG_STREAM)
     customStreamDebug
@@ -3909,23 +3882,13 @@ static int numericCompare(const QVariant::Private *d1, const QVariant::Private *
 /*!
     \internal
  */
-bool QVariant::cmp(const QVariant &v) const
+bool QVariant::equals(const QVariant &v) const
 {
-    auto cmp_helper = [](const QVariant::Private &d1, const QVariant::Private &d2) {
-        Q_ASSERT(d1.type() == d2.type());
-        auto metatype = d1.type();
-        if (metatype.id() >= QMetaType::User)
-            return metatype.equals(QT_PREPEND_NAMESPACE(constData(d1)), QT_PREPEND_NAMESPACE(constData(d2)));
-        return handlerManager[d1.type().id()]->compare(&d1, &d2);
-    };
-
+    auto metatype = d.type();
     // try numerics first, with C++ type promotion rules (no conversion)
-    if (qIsNumericType(d.type().id()) && qIsNumericType(v.d.type().id()))
+    if (qIsNumericType(metatype.id()) && qIsNumericType(v.d.type().id()))
         return numericCompare(&d, &v.d) == 0;
 
-    if (d.type() == v.d.type())
-        return cmp_helper(d, v.d);
-
     QVariant v1 = *this;
     QVariant v2 = v;
     if (v2.canConvert(v1.d.type().id())) {
@@ -3937,7 +3900,11 @@ bool QVariant::cmp(const QVariant &v) const
         if (!v2.convert(v1.d.type().id()))
             return false;
     }
-    return cmp_helper(v1.d, v2.d);
+    metatype = v1.metaType();
+    // For historical reasons: QVariant() == QVariant()
+    if (!metatype.isValid())
+        return true;
+    return metatype.equals(QT_PREPEND_NAMESPACE(constData(v1.d)), QT_PREPEND_NAMESPACE(constData(v2.d)));
 }
 
 /*!

src/corelib/kernel/qvariant.h

@@ -470,20 +470,18 @@ class Q_CORE_EXPORT QVariant
     };
  public:
     typedef bool (*f_null)(const Private *);
-    typedef bool (*f_compare)(const Private *, const Private *);
     typedef bool (*f_convert)(const QVariant::Private *d, int t, void *, bool *);
     typedef void (*f_debugStream)(QDebug, const QVariant &);
     struct Handler {
         f_null isNull;
-        f_compare compare;
         f_convert convert;
         f_debugStream debugStream;
     };
 
     inline bool operator==(const QVariant &v) const
-    { return cmp(v); }
+    { return equals(v); }
     inline bool operator!=(const QVariant &v) const
-    { return !cmp(v); }
+    { return !equals(v); }
 
 protected:
     friend inline bool operator==(const QVariant &, const QVariantComparisonHelper &);
@@ -501,7 +499,7 @@ public:
 #endif
     Private d;
     void create(int type, const void *copy);
-    bool cmp(const QVariant &other) const;
+    bool equals(const QVariant &other) const;
     bool convert(const int t, void *ptr) const; // ### Qt6: drop const
 
 private:
@@ -584,7 +582,7 @@ private:
 
 inline bool operator==(const QVariant &v1, const QVariantComparisonHelper &v2)
 {
-    return v1.cmp(*v2.v);
+    return v1.equals(*v2.v);
 }
 
 inline bool operator!=(const QVariant &v1, const QVariantComparisonHelper &v2)

src/corelib/kernel/qvariant_p.h

@@ -212,49 +212,6 @@ public:
     }
 };
 
-template<class Filter>
-class QVariantComparator {
-    template<typename T, bool IsAcceptedType = Filter::template Acceptor<T>::IsAccepted>
-    struct FilteredComparator {
-        static bool compare(const QVariant::Private *a, const QVariant::Private *b)
-        {
-            return *v_cast<T>(a) == *v_cast<T>(b);
-        }
-    };
-    template<typename T>
-    struct FilteredComparator<T, /* IsAcceptedType = */ false> {
-        static bool compare(const QVariant::Private *, const QVariant::Private *)
-        {
-            // It is not possible to construct a QVariant containing not fully defined type
-            Q_ASSERT(false);
-            return false;
-        }
-    };
-public:
-    QVariantComparator(const QVariant::Private *a, const QVariant::Private *b)
-        : m_a(a), m_b(b)
-    {
-        Q_ASSERT(a->type() == b->type());
-    }
-
-    template<typename T>
-    bool delegate(const T*)
-    {
-        return FilteredComparator<T>::compare(m_a, m_b);
-    }
-
-    bool delegate(const void*) { Q_ASSERT(false); return true; }
-    bool delegate(const QMetaTypeSwitcher::UnknownType*)
-    {
-        return true; // for historical reason invalid variant == invalid variant
-    }
-    bool delegate(const QMetaTypeSwitcher::NotBuiltinType*) { return false; }
-protected:
-    const QVariant::Private *m_a;
-    const QVariant::Private *m_b;
-};
-
-
 Q_CORE_EXPORT const QVariant::Handler *qcoreVariantHandler();
 
 template<class Filter>

src/gui/kernel/qguivariant.cpp

@@ -121,48 +121,6 @@ static bool isNull(const QVariant::Private *d)
     return QMetaTypeSwitcher::switcher<bool>(isNull, d->type().id(), nullptr);
 }
 
-// This class is a hack that customizes access to QPixmap, QBitmap, QCursor and QIcon
-template<class Filter>
-class QGuiVariantComparator : public QVariantComparator<Filter> {
-    typedef QVariantComparator<Filter> Base;
-public:
-    QGuiVariantComparator(const QVariant::Private *a, const QVariant::Private *b)
-        : QVariantComparator<Filter>(a, b)
-    {}
-    template<typename T>
-    bool delegate(const T *p)
-    {
-        return Base::delegate(p);
-    }
-    bool delegate(const QPixmap*)
-    {
-        return v_cast<QPixmap>(Base::m_a)->cacheKey() == v_cast<QPixmap>(Base::m_b)->cacheKey();
-    }
-    bool delegate(const QBitmap*)
-    {
-        return v_cast<QBitmap>(Base::m_a)->cacheKey() == v_cast<QBitmap>(Base::m_b)->cacheKey();
-    }
-#ifndef QT_NO_CURSOR
-    bool delegate(const QCursor*)
-    {
-        return v_cast<QCursor>(Base::m_a)->shape() == v_cast<QCursor>(Base::m_b)->shape();
-    }
-#endif
-#ifndef QT_NO_ICON
-    bool delegate(const QIcon *)
-    {
-        return v_cast<QIcon>(Base::m_a)->cacheKey() == v_cast<QIcon>(Base::m_b)->cacheKey();
-    }
-#endif
-    bool delegate(const void *p) { return Base::delegate(p); }
-};
-
-static bool compare(const QVariant::Private *a, const QVariant::Private *b)
-{
-    QGuiVariantComparator<GuiTypesFilter> comparator(a, b);
-    return QMetaTypeSwitcher::switcher<bool>(comparator, a->type().id(), nullptr);
-}
-
 static bool convert(const QVariant::Private *d, int t,
                  void *result, bool *ok)
 {
@@ -306,7 +264,6 @@ static void streamDebug(QDebug dbg, const QVariant &v)
 
 const QVariant::Handler qt_gui_variant_handler = {
     isNull,
-    compare,
     convert,
 #if !defined(QT_NO_DEBUG_STREAM)
     streamDebug

src/widgets/kernel/qwidgetsvariant.cpp

@@ -54,18 +54,6 @@ static bool isNull(const QVariant::Private *)
     return false;
 }
 
-static bool compare(const QVariant::Private *a, const QVariant::Private *b)
-{
-    Q_ASSERT(a->type() == b->type());
-    switch (a->type().id()) {
-    case QMetaType::QSizePolicy:
-        return *v_cast<QSizePolicy>(a) == *v_cast<QSizePolicy>(b);
-    default:
-        Q_ASSERT(false);
-    }
-    return false;
-}
-
 static bool convert(const QVariant::Private *d, int type, void *result, bool *ok)
 {
     Q_UNUSED(d);
@@ -92,7 +80,6 @@ static void streamDebug(QDebug dbg, const QVariant &v)
 
 static const QVariant::Handler widgets_handler = {
     isNull,
-    compare,
     convert,
 #if !defined(QT_NO_DEBUG_STREAM)
     streamDebug

tests/auto/corelib/kernel/qvariant/tst_qvariant.cpp

@@ -187,7 +187,6 @@ private slots:
 
     void operator_eq_eq_data();
     void operator_eq_eq();
-    void operator_eq_eq_rhs();
 
     void typeName_data();
     void typeName();
@@ -1676,22 +1675,6 @@ void tst_QVariant::operator_eq_eq()
     QCOMPARE( left == right, equal );
 }
 
-void tst_QVariant::operator_eq_eq_rhs()
-{
-    QVariant v = 42;
-
-    QVERIFY(v == 42);
-    QVERIFY(42 == v);
-
-#if 0
-    /* This should _not_ compile */
-    QStringList list;
-    QDateTime dt;
-
-    QVERIFY(dt == list);
-#endif
-}
-
 void tst_QVariant::typeName_data()
 {
     QTest::addColumn<int>("type");

tests/auto/gui/kernel/qguivariant/test/CMakeLists.txt

@@ -14,17 +14,6 @@ qt_add_test(tst_qguivariant
 )
 
 # Resources:
-set(tst_qguivariant_resource_files
-    "black.png"
-    "black2.png"
-)
-
-qt_add_resource(tst_qguivariant "tst_qguivariant"
-    PREFIX
-        "/"
-    FILES
-        ${tst_qguivariant_resource_files}
-)
 set(qguivariant_resource_files
     "data"
 )

tests/auto/gui/kernel/qguivariant/test/test.pro

@@ -1,7 +1,6 @@
 CONFIG += testcase
 TARGET = tst_qguivariant
 SOURCES  += tst_qguivariant.cpp
-RESOURCES = tst_qguivariant.qrc
 INCLUDEPATH += $$PWD/../../../../other/qvariant_common
 QT += testlib
 RESOURCES += qguivariant.qrc

tests/auto/gui/kernel/qguivariant/test/tst_qguivariant.cpp

@@ -109,7 +109,6 @@ private slots:
 
     void guiVariantAtExit();
 
-    void iconEquality();
     void qt4QPolygonFDataStream();
 };
 
@@ -740,33 +739,6 @@ void tst_QGuiVariant::guiVariantAtExit()
     QVERIFY(true);
 }
 
-void tst_QGuiVariant::iconEquality()
-{
-    QIcon i;
-    QVariant a = i;
-    QVariant b = i;
-    QCOMPARE(a, b);
-
-    i = QIcon(":/black.png");
-    a = i;
-    QVERIFY(a != b);
-
-    b = a;
-    QCOMPARE(a, b);
-
-    i = QIcon(":/black2.png");
-    a = i;
-    QVERIFY(a != b);
-
-    b = i;
-    QCOMPARE(a, b);
-
-    // This is a "different" QIcon
-    // even if the contents are the same
-    b = QIcon(":/black2.png");
-    QVERIFY(a != b);
-}
-
 void tst_QGuiVariant::qt4QPolygonFDataStream()
 {
     qRegisterMetaTypeStreamOperators<QPolygonF>();

tests/auto/gui/kernel/qguivariant/test/tst_qguivariant.qrc

@@ -1,6 +0,0 @@
-<!DOCTYPE RCC><RCC version="1.0">
-<qresource prefix="/">
-<file>black.png</file>
-<file>black2.png</file>
-</qresource>
-</RCC>

tests/auto/widgets/itemviews/qtreewidget/tst_qtreewidget.cpp

@@ -1776,7 +1776,10 @@ void tst_QTreeWidget::setData()
                     QCOMPARE(qvariant_cast<QTreeWidgetItem*>(args.at(0)), item);
                     QCOMPARE(qvariant_cast<int>(args.at(1)), j);
                     item->setIcon(j, icon);
-                    QCOMPARE(itemChangedSpy.count(), 0);
+                    QCOMPARE(itemChangedSpy.count(), 1);
+                    args = itemChangedSpy.takeFirst();
+                    QCOMPARE(qvariant_cast<QTreeWidgetItem*>(args.at(0)), item);
+                    QCOMPARE(qvariant_cast<int>(args.at(1)), j);
 
                     const QString toolTip = QLatin1String("toolTip ") + iS;
                     item->setToolTip(j, toolTip);