QVariant: make a major simplification in the numeric comparison

The code implementing the C++ rules of type promotion and conversion
was too pedantic. There's no need to follow the letter of the standard,
not when we can now assume that everything is two's complement (this was
true for all architectures we supported when I wrote this code in 2014,
but wasn't required by the standard).

So we can reduce this to fewer comparisons and fewer rules, using the
size of the type, not just the type ID.

Change-Id: I3d74c753055744deb8acfffd172446b02444c0c0
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

3 files changed, 233 insertions, 49 deletions

diff --git a/src/corelib/kernel/qvariant.cpp b/src/corelib/kernel/qvariant.cpp
index 22f2625b32..a8c5d756c7 100644
--- a/src/corelib/kernel/qvariant.cpp
+++ b/src/corelib/kernel/qvariant.cpp
@@ -2167,28 +2167,13 @@ static bool qIsFloatingPoint(uint tp)
     return tp == QMetaType::Double || tp == QMetaType::Float;
 }
 
-static int normalizeLowerRanks(uint tp)
-{
-    static const qulonglong numericTypeBits =
-            Q_UINT64_C(1) << QMetaType::Bool |
-            Q_UINT64_C(1) << QMetaType::Char |
-            Q_UINT64_C(1) << QMetaType::SChar |
-            Q_UINT64_C(1) << QMetaType::UChar |
-            Q_UINT64_C(1) << QMetaType::Short |
-            Q_UINT64_C(1) << QMetaType::UShort;
-    return numericTypeBits & (Q_UINT64_C(1) << tp) ? uint(QMetaType::Int) : tp;
-}
-
-static int normalizeLong(uint tp)
-{
-    const uint IntType = sizeof(long) == sizeof(int) ? QMetaType::Int : QMetaType::LongLong;
-    const uint UIntType = sizeof(ulong) == sizeof(uint) ? QMetaType::UInt : QMetaType::ULongLong;
-    return tp == QMetaType::Long ? IntType :
-           tp == QMetaType::ULong ? UIntType : tp;
-}
-
-static int numericTypePromotion(uint t1, uint t2)
+static int numericTypePromotion(const QtPrivate::QMetaTypeInterface *iface1,
+                                const QtPrivate::QMetaTypeInterface *iface2)
 {
+    // We don't need QMetaType::id() here because the type Id is always stored
+    // directly for the types we're comparing against below.
+    uint t1 = iface1->typeId;
+    uint t2 = iface2->typeId;
     Q_ASSERT(qIsNumericType(t1));
     Q_ASSERT(qIsNumericType(t2));
 
@@ -2214,26 +2199,30 @@ static int numericTypePromotion(uint t1, uint t2)
     if (qIsFloatingPoint(t1) || qIsFloatingPoint(t2))
         return QMetaType::QReal;
 
+    auto isUnsigned = [](uint tp) {
+        // only types for which sizeof(T) >= sizeof(int); lesser ones promote to int
+        return tp == QMetaType::ULongLong || tp == QMetaType::ULong ||
+                tp == QMetaType::UInt;
+    };
+    bool isUnsigned1 = isUnsigned(t1);
+    bool isUnsigned2 = isUnsigned(t2);
+
     // integral rules:
-    // for all platforms we support, int can always hold the values of lower-ranked types
-    t1 = normalizeLowerRanks(t1);
-    t2 = normalizeLowerRanks(t2);
-
-    // normalize long / ulong: in all platforms we run, they're either the same as int or as long long
-    t1 = normalizeLong(t1);
-    t2 = normalizeLong(t2);
-
-    // implement the other rules
-    // the four possibilities are Int, UInt, LongLong and ULongLong
-    // if any of the two is ULongLong, then it wins (highest rank, unsigned)
-    // otherwise, if one of the two is LongLong, then the other is either LongLong too or lower-ranked
-    // otherwise, if one of the two is UInt, then the other is either UInt too or Int
-    if (t1 == QMetaType::ULongLong || t2 == QMetaType::ULongLong)
+    // 1) if either type is a 64-bit unsigned, compare as 64-bit unsigned
+    if (isUnsigned1 && iface1->size > sizeof(int))
+        return QMetaType::ULongLong;
+    if (isUnsigned2 && iface2->size > sizeof(int))
         return QMetaType::ULongLong;
-    if (t1 == QMetaType::LongLong || t2 == QMetaType::LongLong)
+
+    // 2) if either type is 64-bit, compare as 64-bit signed
+    if (iface1->size > sizeof(int) || iface2->size > sizeof(int))
         return QMetaType::LongLong;
-    if (t1 == QMetaType::UInt || t2 == QMetaType::UInt)
+
+    // 3) if either type is 32-bit unsigned, compare as 32-bit unsigned
+    if (isUnsigned1 || isUnsigned2)
         return QMetaType::UInt;
+
+    // 4) otherwise, just do int promotion
     return QMetaType::Int;
 }
 
@@ -2248,12 +2237,9 @@ static bool integralEquals(uint promotedType, const QVariant::Private *d1, const
         return int(*l1) == int(*l2);
     if (promotedType == QMetaType::UInt)
         return uint(*l1) == uint(*l2);
-    if (promotedType == QMetaType::LongLong)
-        return l1 == l2;
     if (promotedType == QMetaType::ULongLong)
         return qulonglong(*l1) == qulonglong(*l2);
-
-    Q_UNREACHABLE_RETURN(0);
+    return l1 == l2;
 }
 
 namespace {
@@ -2281,11 +2267,6 @@ static std::optional<int> integralCompare(uint promotedType, const QVariant::Pri
     std::optional<qlonglong> l2 = qConvertToNumber(d2, promotedType == QMetaType::Bool);
     if (!l1 || !l2)
         return std::nullopt;
-
-    if (promotedType == QMetaType::Bool)
-        return spaceShip<bool>(*l1, *l2);
-    if (promotedType == QMetaType::Int)
-        return spaceShip<int>(*l1, *l2);
     if (promotedType == QMetaType::UInt)
         return spaceShip<uint>(*l1, *l2);
     if (promotedType == QMetaType::LongLong)
@@ -2293,12 +2274,12 @@ static std::optional<int> integralCompare(uint promotedType, const QVariant::Pri
     if (promotedType == QMetaType::ULongLong)
         return spaceShip<qulonglong>(*l1, *l2);
 
-    Q_UNREACHABLE_RETURN(0);
+    return spaceShip<int>(*l1, *l2);
 }
 
 static std::optional<int> numericCompare(const QVariant::Private *d1, const QVariant::Private *d2)
 {
-    uint promotedType = numericTypePromotion(d1->type().id(), d2->type().id());
+    uint promotedType = numericTypePromotion(d1->typeInterface(), d2->typeInterface());
     if (promotedType != QMetaType::QReal)
         return integralCompare(promotedType, d1, d2);
 
@@ -2317,7 +2298,7 @@ static std::optional<int> numericCompare(const QVariant::Private *d1, const QVar
 
 static bool numericEquals(const QVariant::Private *d1, const QVariant::Private *d2)
 {
-    uint promotedType = numericTypePromotion(d1->type().id(), d2->type().id());
+    uint promotedType = numericTypePromotion(d1->typeInterface(), d2->typeInterface());
     if (promotedType != QMetaType::QReal)
         return integralEquals(promotedType, d1, d2);
 
diff --git a/src/testlib/qtest.h b/src/testlib/qtest.h
index 7b845a61a9..f87995e0db 100644
--- a/src/testlib/qtest.h
+++ b/src/testlib/qtest.h
@@ -207,6 +207,19 @@ template<> inline char *toString(const QVariant &v)
     return qstrdup(vstring.constData());
 }
 
+template<> inline char *toString(const QPartialOrdering &o)
+{
+    if (o == QPartialOrdering::Less)
+        return qstrdup("Less");
+    if (o == QPartialOrdering::Equivalent)
+        return qstrdup("Equivalent");
+    if (o == QPartialOrdering::Greater)
+        return qstrdup("Greater");
+    if (o == QPartialOrdering::Unordered)
+        return qstrdup("Unordered");
+    return qstrdup("<invalid>");
+}
+
 namespace Internal {
 struct QCborValueFormatter
 {
diff --git a/tests/auto/corelib/kernel/qvariant/tst_qvariant.cpp b/tests/auto/corelib/kernel/qvariant/tst_qvariant.cpp
index 5f8f965973..e7579a7927 100644
--- a/tests/auto/corelib/kernel/qvariant/tst_qvariant.cpp
+++ b/tests/auto/corelib/kernel/qvariant/tst_qvariant.cpp
@@ -209,6 +209,8 @@ private slots:
     void variantHash();
 
     void convertToQUint8() const;
+    void compareNumerics_data() const;
+    void compareNumerics() const;
     void comparePointers() const;
     void voidStar() const;
     void dataStar() const;
@@ -2714,6 +2716,194 @@ void tst_QVariant::convertToQUint8() const
     }
 }
 
+void tst_QVariant::compareNumerics_data() const
+{
+    QTest::addColumn<QVariant>("v1");
+    QTest::addColumn<QVariant>("v2");
+    QTest::addColumn<QPartialOrdering>("result");
+
+    QTest::addRow("invalid-invalid")
+            << QVariant() << QVariant() << QPartialOrdering::Unordered;
+
+    static const auto asString = [](const QVariant &v) {
+        if (v.isNull())
+            return QStringLiteral("null");
+        switch (v.typeId()) {
+        case QMetaType::Char:
+        case QMetaType::Char16:
+        case QMetaType::Char32:
+        case QMetaType::UChar:
+            return QString::number(v.toUInt());
+        case QMetaType::SChar:
+            return QString::number(v.toInt());
+        }
+        return v.toString();
+    };
+
+    auto addCompareToInvalid = [](auto value) {
+        QVariant v = QVariant::fromValue(value);
+        QTest::addRow("invalid-%s(%s)", v.typeName(), qPrintable(asString(v)))
+                << QVariant() << v << QPartialOrdering::Unordered;
+        QTest::addRow("%s(%s)-invalid", v.typeName(), qPrintable(asString(v)))
+                << v << QVariant() << QPartialOrdering::Unordered;
+    };
+    addCompareToInvalid(false);
+    addCompareToInvalid(true);
+    addCompareToInvalid(char(0));
+    addCompareToInvalid(qint8(0));
+    addCompareToInvalid(quint8(0));
+    addCompareToInvalid(short(0));
+    addCompareToInvalid(ushort(0));
+    addCompareToInvalid(int(0));
+    addCompareToInvalid(uint(0));
+    addCompareToInvalid(long(0));
+    addCompareToInvalid(ulong(0));
+    addCompareToInvalid(qint64(0));
+    addCompareToInvalid(quint64(0));
+    addCompareToInvalid(0.f);
+    addCompareToInvalid(0.0);
+    addCompareToInvalid(QCborSimpleType{});
+
+QT_WARNING_PUSH
+QT_WARNING_DISABLE_CLANG("-Wsign-compare")
+QT_WARNING_DISABLE_GCC("-Wsign-compare")
+QT_WARNING_DISABLE_MSVC(4018)   // '<': signed/unsigned mismatch
+    static const auto addComparePair = [](auto value1, auto value2) {
+        QVariant v1 = QVariant::fromValue(value1);
+        QVariant v2 = QVariant::fromValue(value2);
+        QPartialOrdering order = QPartialOrdering::Unordered;
+        if (value1 == value2)
+            order = QPartialOrdering::Equivalent;
+        else if (value1 < value2)
+            order = QPartialOrdering::Less;
+        else if (value1 > value2)
+            order = QPartialOrdering::Greater;
+        QTest::addRow("%s(%s)-%s(%s)", v1.typeName(), qPrintable(asString(v1)),
+                      v2.typeName(), qPrintable(asString(v2)))
+                << v1 << v2 << order;
+    };
+QT_WARNING_POP
+
+    // homogeneous first
+    static const auto addList = [](auto list) {
+        for (auto v1 : list)
+            for (auto v2 : list)
+                addComparePair(v1, v2);
+    };
+
+    auto addSingleType = [](auto zero) {
+        using T = decltype(zero);
+        T one = T(zero + 1);
+        T min = std::numeric_limits<T>::min();
+        T max = std::numeric_limits<T>::max();
+        T mid = max / 2 + 1;
+        if (min != zero)
+            addList(std::array{zero, one, min, mid, max});
+        else
+            addList(std::array{zero, one, mid, max});
+    };
+    addList(std::array{ false, true });
+    addList(std::array{ QCborSimpleType{}, QCborSimpleType::False, QCborSimpleType(0xff) });
+    addSingleType(char(0));
+    addSingleType(qint8(0));
+    addSingleType(quint8(0));
+    addSingleType(qint16(0));
+    addSingleType(quint16(0));
+    addSingleType(qint32(0));
+    addSingleType(quint32(0));
+    addSingleType(qint64(0));
+    addSingleType(quint64(0));
+    addSingleType(0.f);
+    addSingleType(0.0);
+
+    // heterogeneous
+    addComparePair(char(0), qint8(-127));
+    addComparePair(char(127), qint8(127));
+    addComparePair(char(127), quint8(127));
+    addComparePair(qint8(-1), quint8(255));
+    addComparePair(0U, -1);
+    addComparePair(~0U, -1);
+    addComparePair(Q_UINT64_C(0), -1);
+    addComparePair(~Q_UINT64_C(0), -1);
+    addComparePair(Q_UINT64_C(0), Q_INT64_C(-1));
+    addComparePair(~Q_UINT64_C(0), Q_INT64_C(-1));
+    addComparePair(INT_MAX, uint(INT_MAX));
+    addComparePair(INT_MAX, qint64(INT_MAX) + 1);
+    addComparePair(INT_MAX, UINT_MAX);
+    addComparePair(INT_MAX, qint64(UINT_MAX));
+    addComparePair(INT_MAX, qint64(UINT_MAX) + 1);
+    addComparePair(INT_MAX, quint64(UINT_MAX));
+    addComparePair(INT_MAX, quint64(UINT_MAX) + 1);
+    addComparePair(INT_MAX, LONG_MIN);
+    addComparePair(INT_MAX, LONG_MAX);
+    addComparePair(INT_MAX, LLONG_MIN);
+    addComparePair(INT_MAX, LLONG_MAX);
+    addComparePair(INT_MIN, uint(INT_MIN));
+    addComparePair(INT_MIN, uint(INT_MIN) + 1);
+    addComparePair(INT_MIN + 1, uint(INT_MIN));
+    addComparePair(INT_MIN + 1, uint(INT_MIN) + 1);
+    addComparePair(INT_MIN, qint64(INT_MIN) - 1);
+    addComparePair(INT_MIN + 1, qint64(INT_MIN) + 1);
+    addComparePair(INT_MIN + 1, qint64(INT_MIN) - 1);
+    addComparePair(INT_MIN, UINT_MAX);
+    addComparePair(INT_MIN, qint64(UINT_MAX));
+    addComparePair(INT_MIN, qint64(UINT_MAX) + 1);
+    addComparePair(INT_MIN, quint64(UINT_MAX));
+    addComparePair(INT_MIN, quint64(UINT_MAX) + 1);
+    addComparePair(UINT_MAX, qint64(UINT_MAX) + 1);
+    addComparePair(UINT_MAX, quint64(UINT_MAX) + 1);
+    addComparePair(UINT_MAX, qint64(INT_MIN) - 1);
+    addComparePair(UINT_MAX, quint64(INT_MIN) + 1);
+    addComparePair(LLONG_MAX, quint64(LLONG_MAX));
+    addComparePair(LLONG_MAX, quint64(LLONG_MAX) + 1);
+    addComparePair(LLONG_MIN, quint64(LLONG_MAX));
+    addComparePair(LLONG_MIN, quint64(LLONG_MAX) + 1);
+    addComparePair(LLONG_MIN, quint64(LLONG_MIN) + 1);
+    addComparePair(LLONG_MIN + 1, quint64(LLONG_MIN) + 1);
+    addComparePair(LLONG_MIN, LLONG_MAX - 1);
+    addComparePair(LLONG_MIN, LLONG_MAX);
+
+    // floating point
+    addComparePair(0.f, 0);
+    addComparePair(0.f, 0U);
+    addComparePair(0.f, Q_INT64_C(0));
+    addComparePair(0.f, Q_UINT64_C(0));
+    addComparePair(0.f, 0.);
+    addComparePair(0.f, 1.);
+    addComparePair(0.f, 1.);
+    addComparePair(float(1 << 24), 1 << 24);
+    addComparePair(float(1 << 24) - 1, (1 << 24) - 1);
+    addComparePair(-float(1 << 24), 1 << 24);
+    addComparePair(-float(1 << 24) + 1, -(1 << 24) + 1);
+    addComparePair(HUGE_VALF, qInf());
+    addComparePair(HUGE_VALF, -qInf());
+    addComparePair(qQNaN(), std::numeric_limits<float>::quiet_NaN());
+    if (sizeof(qreal) == sizeof(double)) {
+        addComparePair(std::numeric_limits<float>::min(), std::numeric_limits<double>::min());
+        addComparePair(std::numeric_limits<float>::min(), std::numeric_limits<double>::min());
+        addComparePair(std::numeric_limits<float>::max(), std::numeric_limits<double>::min());
+        addComparePair(std::numeric_limits<float>::max(), std::numeric_limits<double>::max());
+        addComparePair(double(Q_INT64_C(1) << 53), Q_INT64_C(1) << 53);
+        addComparePair(double(Q_INT64_C(1) << 53) - 1, (Q_INT64_C(1) << 53) - 1);
+        addComparePair(-double(Q_INT64_C(1) << 53), Q_INT64_C(1) << 53);
+        addComparePair(-double(Q_INT64_C(1) << 53) + 1, (Q_INT64_C(1) << 53) + 1);
+    }
+}
+
+void tst_QVariant::compareNumerics() const
+{
+    QFETCH(QVariant, v1);
+    QFETCH(QVariant, v2);
+    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);
+}
+
 void tst_QVariant::comparePointers() const
 {
     class MyClass