From 393d5efda30aac8f20c35dc22b7d22c350f6f096 Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Fri, 4 Nov 2022 15:15:26 -0700 Subject: [PATCH] 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 --- src/corelib/kernel/qvariant.cpp | 79 +++----- src/testlib/qtest.h | 13 ++ .../corelib/kernel/qvariant/tst_qvariant.cpp | 190 ++++++++++++++++++ 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 (t1 == QMetaType::LongLong || t2 == QMetaType::LongLong) + if (isUnsigned2 && iface2->size > sizeof(int)) + return QMetaType::ULongLong; + + // 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 integralCompare(uint promotedType, const QVariant::Pri std::optional l2 = qConvertToNumber(d2, promotedType == QMetaType::Bool); if (!l1 || !l2) return std::nullopt; - - if (promotedType == QMetaType::Bool) - return spaceShip(*l1, *l2); - if (promotedType == QMetaType::Int) - return spaceShip(*l1, *l2); if (promotedType == QMetaType::UInt) return spaceShip(*l1, *l2); if (promotedType == QMetaType::LongLong) @@ -2293,12 +2274,12 @@ static std::optional integralCompare(uint promotedType, const QVariant::Pri if (promotedType == QMetaType::ULongLong) return spaceShip(*l1, *l2); - Q_UNREACHABLE_RETURN(0); + return spaceShip(*l1, *l2); } static std::optional 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 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(""); +} + 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("v1"); + QTest::addColumn("v2"); + QTest::addColumn("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::min(); + T max = std::numeric_limits::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::quiet_NaN()); + if (sizeof(qreal) == sizeof(double)) { + addComparePair(std::numeric_limits::min(), std::numeric_limits::min()); + addComparePair(std::numeric_limits::min(), std::numeric_limits::min()); + addComparePair(std::numeric_limits::max(), std::numeric_limits::min()); + addComparePair(std::numeric_limits::max(), std::numeric_limits::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