From 524edc7363ecb2ea2697ab17409705c277664e54 Mon Sep 17 00:00:00 2001 From: Lars Knoll Date: Sun, 12 Jul 2020 22:27:41 +0200 Subject: [PATCH] Clean up QVariant::Private::Data Remove all the internal members of the union. Instead replace it with raw storage (uchar[]) aligned to max_align_t. Place all accesses to the internal members with get<> methods for consistency. Change-Id: Icebf46b90c9375aa6ea0b5913b2132608e8c223d Reviewed-by: Maurice Kalinowski --- src/corelib/kernel/qvariant.cpp | 143 ++++++++---------- src/corelib/kernel/qvariant.h | 58 +++---- .../corelib/kernel/qvariant/tst_qvariant.cpp | 4 +- 3 files changed, 90 insertions(+), 115 deletions(-) diff --git a/src/corelib/kernel/qvariant.cpp b/src/corelib/kernel/qvariant.cpp index 2a0066c9e2..fa021dc1d1 100644 --- a/src/corelib/kernel/qvariant.cpp +++ b/src/corelib/kernel/qvariant.cpp @@ -107,21 +107,21 @@ static qlonglong qMetaTypeNumber(const QVariant::Private *d) { switch (d->type().id()) { case QMetaType::Int: - return d->data.i; + return d->get(); case QMetaType::LongLong: - return d->data.ll; + return d->get(); case QMetaType::Char: - return qlonglong(d->data.c); + return qlonglong(d->get()); case QMetaType::SChar: - return qlonglong(d->data.sc); + return qlonglong(d->get()); case QMetaType::Short: - return qlonglong(d->data.s); + return qlonglong(d->get()); case QMetaType::Long: - return qlonglong(d->data.l); + return qlonglong(d->get()); case QMetaType::Float: - return qRound64(d->data.f); + return qRound64(d->get()); case QMetaType::Double: - return qRound64(d->data.d); + return qRound64(d->get()); #ifndef QT_BOOTSTRAPPED case QMetaType::QJsonValue: return d->get().toDouble(); @@ -137,15 +137,15 @@ static qulonglong qMetaTypeUNumber(const QVariant::Private *d) { switch (d->type().id()) { case QMetaType::UInt: - return d->data.u; + return d->get(); case QMetaType::ULongLong: - return d->data.ull; + return d->get(); case QMetaType::UChar: - return d->data.uc; + return d->get(); case QMetaType::UShort: - return d->data.us; + return d->get(); case QMetaType::ULong: - return d->data.ul; + return d->get(); } Q_ASSERT(false); return 0; @@ -178,7 +178,7 @@ static qlonglong qConvertToNumber(const QVariant::Private *d, bool *ok, bool all case QMetaType::QByteArray: return d->get().toLongLong(ok); case QMetaType::Bool: - return qlonglong(d->data.b); + return qlonglong(d->get()); #ifndef QT_BOOTSTRAPPED case QMetaType::QCborValue: if (!d->get().isInteger() && !d->get().isDouble()) @@ -212,13 +212,13 @@ static qlonglong qConvertToNumber(const QVariant::Private *d, bool *ok, bool all || d->type().id() == QMetaType::QCborSimpleType) { switch (typeInfo.sizeOf()) { case 1: - return d->is_shared ? *reinterpret_cast(d->data.shared->data()) : d->data.sc; + return d->get(); case 2: - return d->is_shared ? *reinterpret_cast(d->data.shared->data()) : d->data.s; + return d->get(); case 4: - return d->is_shared ? *reinterpret_cast(d->data.shared->data()) : d->data.i; + return d->get(); case 8: - return d->is_shared ? *reinterpret_cast(d->data.shared->data()) : d->data.ll; + return d->get(); } } @@ -233,9 +233,9 @@ static qreal qConvertToRealNumber(const QVariant::Private *d, bool *ok) case QMetaType::QString: return d->get().toDouble(ok); case QMetaType::Double: - return qreal(d->data.d); + return qreal(d->get()); case QMetaType::Float: - return qreal(d->data.f); + return qreal(d->get()); case QMetaType::ULongLong: case QMetaType::UInt: case QMetaType::UChar: @@ -266,7 +266,7 @@ static qulonglong qConvertToUnsignedNumber(const QVariant::Private *d, bool *ok) case QMetaType::QByteArray: return d->get().toULongLong(ok); case QMetaType::Bool: - return qulonglong(d->data.b); + return qulonglong(d->get()); #ifndef QT_BOOTSTRAPPED case QMetaType::QCborValue: if (d->get().isDouble()) @@ -300,13 +300,13 @@ static qulonglong qConvertToUnsignedNumber(const QVariant::Private *d, bool *ok) if (typeInfo.flags() & QMetaType::IsEnumeration) { switch (typeInfo.sizeOf()) { case 1: - return d->is_shared ? *reinterpret_cast(d->data.shared->data()) : d->data.uc; + return d->get(); case 2: - return d->is_shared ? *reinterpret_cast(d->data.shared->data()) : d->data.us; + return d->get(); case 4: - return d->is_shared ? *reinterpret_cast(d->data.shared->data()) : d->data.u; + return d->get(); case 8: - return d->is_shared ? *reinterpret_cast(d->data.shared->data()) : d->data.ull; + return d->get(); } } @@ -321,16 +321,6 @@ inline bool qt_convertToBool(const QVariant::Private *const d) return !(str.isEmpty() || str == LiteralWrapper("0") || str == LiteralWrapper("false")); } -/*! - \internal - Returns the internal data pointer from \a d. - */ - -static const void *constData(const QVariant::Private &d) -{ - return d.is_shared ? d.data.shared->data() : reinterpret_cast(&d.data.c); -} - #ifndef QT_NO_QOBJECT /*! \internal @@ -361,7 +351,7 @@ static bool convert(const QVariant::Private *d, int t, void *result) Q_ASSERT(result); if (d->type().id() >= QMetaType::LastCoreType || t >= QMetaType::LastCoreType) { - if (QMetaType::convert(constData(*d), d->type().id(), result, t)) + if (QMetaType::convert(d->storage(), d->type().id(), result, t)) return true; } @@ -414,7 +404,7 @@ static bool convert(const QVariant::Private *d, int t, void *result) case QMetaType::Char: case QMetaType::SChar: case QMetaType::UChar: - *str = QChar::fromLatin1(d->data.c); + *str = QChar::fromLatin1(d->get()); break; case QMetaType::Short: case QMetaType::Long: @@ -429,10 +419,10 @@ static bool convert(const QVariant::Private *d, int t, void *result) *str = QString::number(qMetaTypeUNumber(d)); break; case QMetaType::Float: - *str = QString::number(d->data.f, 'g', QLocale::FloatingPointShortest); + *str = QString::number(d->get(), 'g', QLocale::FloatingPointShortest); break; case QMetaType::Double: - *str = QString::number(d->data.d, 'g', QLocale::FloatingPointShortest); + *str = QString::number(d->get(), 'g', QLocale::FloatingPointShortest); break; #if QT_CONFIG(datestring) case QMetaType::QDate: @@ -446,7 +436,7 @@ static bool convert(const QVariant::Private *d, int t, void *result) break; #endif case QMetaType::Bool: - *str = d->data.b ? QStringLiteral("true") : QStringLiteral("false"); + *str = d->get() ? QStringLiteral("true") : QStringLiteral("false"); break; case QMetaType::QByteArray: *str = QString::fromUtf8(d->get().constData()); @@ -639,15 +629,15 @@ static bool convert(const QVariant::Private *d, int t, void *result) *ba = d->get().toUtf8(); break; case QMetaType::Double: - *ba = QByteArray::number(d->data.d, 'g', QLocale::FloatingPointShortest); + *ba = QByteArray::number(d->get(), 'g', QLocale::FloatingPointShortest); break; case QMetaType::Float: - *ba = QByteArray::number(d->data.f, 'g', QLocale::FloatingPointShortest); + *ba = QByteArray::number(d->get(), 'g', QLocale::FloatingPointShortest); break; case QMetaType::Char: case QMetaType::SChar: case QMetaType::UChar: - *ba = QByteArray(1, d->data.c); + *ba = QByteArray(1, d->get()); break; case QMetaType::Int: case QMetaType::LongLong: @@ -662,7 +652,7 @@ static bool convert(const QVariant::Private *d, int t, void *result) *ba = QByteArray::number(qMetaTypeUNumber(d)); break; case QMetaType::Bool: - *ba = QByteArray(d->data.b ? "true" : "false"); + *ba = QByteArray(d->get() ? "true" : "false"); break; case QMetaType::QUuid: *ba = d->get().toByteArray(); @@ -783,10 +773,10 @@ static bool convert(const QVariant::Private *d, int t, void *result) *f = d->get().toDouble(&ok); return ok; case QMetaType::Bool: - *f = double(d->data.b); + *f = double(d->get()); break; case QMetaType::Float: - *f = double(d->data.f); + *f = double(d->get()); break; case QMetaType::LongLong: case QMetaType::Int: @@ -831,10 +821,10 @@ static bool convert(const QVariant::Private *d, int t, void *result) *f = d->get().toFloat(&ok); return ok; case QMetaType::Bool: - *f = float(d->data.b); + *f = float(d->get()); break; case QMetaType::Double: - *f = float(d->data.d); + *f = float(d->get()); break; case QMetaType::LongLong: case QMetaType::Int: @@ -1023,7 +1013,7 @@ static bool convert(const QVariant::Private *d, int t, void *result) *static_cast(result) = QJsonValue(QJsonValue::Null); break; case QMetaType::Bool: - *static_cast(result) = QJsonValue(d->data.b); + *static_cast(result) = QJsonValue(d->get()); break; case QMetaType::Int: case QMetaType::UInt: @@ -1153,7 +1143,7 @@ static bool convert(const QVariant::Private *d, int t, void *result) *static_cast(result) = QCborValue(QCborValue::Null); break; case QMetaType::Bool: - *static_cast(result) = QCborValue(d->data.b); + *static_cast(result) = QCborValue(d->get()); break; case QMetaType::Int: case QMetaType::UInt: @@ -1366,8 +1356,7 @@ static void customConstruct(QVariant::Private *d, const void *copy) return; } - // this logic should match with QVariantIntegrator::CanUseInternalSpace - if (size <= sizeof(QVariant::Private::Data)) { + if (QVariant::Private::canUseInternalSpace(size)) { type.construct(&d->data, copy); d->is_shared = false; } else { @@ -1929,25 +1918,25 @@ QVariant::QVariant(QMetaType type, const void *copy) : d(type) QVariant::QVariant(int val) : d(Int) -{ d.data.i = val; } +{ d.set(val); } QVariant::QVariant(uint val) : d(UInt) -{ d.data.u = val; } +{ d.set(val); } QVariant::QVariant(qlonglong val) : d(LongLong) -{ d.data.ll = val; } +{ d.set(val); } QVariant::QVariant(qulonglong val) : d(ULongLong) -{ d.data.ull = val; } +{ d.set(val); } QVariant::QVariant(bool val) : d(Bool) -{ d.data.b = val; } +{ d.set(val); } QVariant::QVariant(double val) : d(Double) -{ d.data.d = val; } +{ d.set(val); } QVariant::QVariant(float val) : d(QMetaType::Float) -{ d.data.f = val; } +{ d.set(val); } QVariant::QVariant(const QByteArray &val) : d(ByteArray) @@ -2474,7 +2463,7 @@ inline T qVariantToHelper(const QVariant::Private &d) T ret; if (d.type().id() >= QMetaType::LastCoreType || targetType.id() >= QMetaType::LastCoreType) { - const void * const from = constData(d); + const void * const from = d.storage(); if (QMetaType::convert(from, d.type().id(), &ret, targetType.id())) return ret; } @@ -2908,7 +2897,7 @@ inline T qNumVariantToHelper(const QVariant::Private &d, bool *ok, const T& val) T ret = 0; if ((d.type().id() >= QMetaType::LastCoreType || t >= QMetaType::LastCoreType) - && QMetaType::convert(constData(d), d.type().id(), &ret, t)) + && QMetaType::convert(d.storage(), d.type().id(), &ret, t)) return ret; bool success = convert(&d, t, &ret); @@ -2936,7 +2925,7 @@ inline T qNumVariantToHelper(const QVariant::Private &d, bool *ok, const T& val) */ int QVariant::toInt(bool *ok) const { - return qNumVariantToHelper(d, ok, d.data.i); + return qNumVariantToHelper(d, ok, d.get()); } /*! @@ -2958,7 +2947,7 @@ int QVariant::toInt(bool *ok) const */ uint QVariant::toUInt(bool *ok) const { - return qNumVariantToHelper(d, ok, d.data.u); + return qNumVariantToHelper(d, ok, d.get()); } /*! @@ -2975,7 +2964,7 @@ uint QVariant::toUInt(bool *ok) const */ qlonglong QVariant::toLongLong(bool *ok) const { - return qNumVariantToHelper(d, ok, d.data.ll); + return qNumVariantToHelper(d, ok, d.get()); } /*! @@ -2992,7 +2981,7 @@ qlonglong QVariant::toLongLong(bool *ok) const */ qulonglong QVariant::toULongLong(bool *ok) const { - return qNumVariantToHelper(d, ok, d.data.ull); + return qNumVariantToHelper(d, ok, d.get()); } /*! @@ -3010,7 +2999,7 @@ qulonglong QVariant::toULongLong(bool *ok) const bool QVariant::toBool() const { if (d.type() == QMetaType::fromType()) - return d.data.b; + return d.get(); bool res = false; @@ -3039,7 +3028,7 @@ bool QVariant::toBool() const */ double QVariant::toDouble(bool *ok) const { - return qNumVariantToHelper(d, ok, d.data.d); + return qNumVariantToHelper(d, ok, d.get()); } /*! @@ -3058,7 +3047,7 @@ double QVariant::toDouble(bool *ok) const */ float QVariant::toFloat(bool *ok) const { - return qNumVariantToHelper(d, ok, d.data.f); + return qNumVariantToHelper(d, ok, d.get()); } /*! @@ -3077,7 +3066,7 @@ float QVariant::toFloat(bool *ok) const */ qreal QVariant::toReal(bool *ok) const { - return qNumVariantToHelper(d, ok, d.data.real); + return qNumVariantToHelper(d, ok, d.get()); } /*! @@ -3356,7 +3345,7 @@ bool QVariant::canConvert(int targetTypeId) const if (QMetaType::typeFlags(targetTypeId) & QMetaType::IsEnumeration) { targetTypeId = QMetaType::Int; } else { - return canConvertMetaObject(currentType, targetTypeId, d.data.o); + return canConvertMetaObject(currentType, targetTypeId, d.get()); } } @@ -3504,7 +3493,7 @@ bool QVariant::canConvert(int targetTypeId) const && qCanConvertMatrix[QVariant::Int] & (1U << currentType)) || QMetaType::typeFlags(currentType) & QMetaType::IsEnumeration; case QMetaType::QObjectStar: - return canConvertMetaObject(currentType, targetTypeId, d.data.o); + return canConvertMetaObject(currentType, targetTypeId, d.get()); default: return false; } @@ -3553,7 +3542,7 @@ bool QVariant::convert(int targetTypeId) return false; if ((QMetaType::typeFlags(oldValue.userType()) & QMetaType::PointerToQObject) && (QMetaType::typeFlags(targetTypeId) & QMetaType::PointerToQObject)) { - create(targetTypeId, &oldValue.d.data.o); + create(targetTypeId, &oldValue.d.get()); return true; } @@ -3821,7 +3810,7 @@ bool QVariant::equals(const QVariant &v) const if (!metatype.isValid()) return true; - return metatype.equals(QT_PREPEND_NAMESPACE(constData(d)), QT_PREPEND_NAMESPACE(constData(v.d))); + return metatype.equals(d.storage(), v.d.storage()); } /*! @@ -3859,10 +3848,8 @@ bool QVariant::isNull() const { if (d.is_null || !metaType().isValid()) return true; - if (metaType().flags() & QMetaType::IsPointer) { - const void *d_ptr = d.is_shared ? d.data.shared->data() : &(d.data); - return *static_cast(d_ptr) == nullptr; - } + if (metaType().flags() & QMetaType::IsPointer) + return d.get() == nullptr; return false; } @@ -3874,7 +3861,7 @@ QDebug operator<<(QDebug dbg, const QVariant &v) dbg.nospace() << "QVariant("; if (typeId != QMetaType::UnknownType) { dbg << QMetaType::typeName(typeId) << ", "; - bool streamed = v.d.type().debugStream(dbg, constData(v.d)); + bool streamed = v.d.type().debugStream(dbg, v.d.storage()); if (!streamed && v.canConvert()) dbg << v.toString(); } else { diff --git a/src/corelib/kernel/qvariant.h b/src/corelib/kernel/qvariant.h index 419aefd081..258dac3bbc 100644 --- a/src/corelib/kernel/qvariant.h +++ b/src/corelib/kernel/qvariant.h @@ -352,7 +352,7 @@ class Q_CORE_EXPORT QVariant void *data(); const void *constData() const - { return d.is_shared ? d.data.shared->data() : &d.data.ptr; } + { return d.storage(); } inline const void *data() const { return constData(); } template, QVariant>>> @@ -442,7 +442,21 @@ class Q_CORE_EXPORT QVariant }; struct Private { - Private() noexcept : packedType(0), is_shared(false), is_null(true) {} + static constexpr size_t MaxInternalSize = 3*sizeof(void *); + template + static constexpr bool CanUseInternalSpace = (sizeof(T) <= MaxInternalSize); + static constexpr bool canUseInternalSpace(size_t s) { return s <= MaxInternalSize; } + + alignas(std::max_align_t) union + { + uchar data[MaxInternalSize] = {}; + PrivateShared *shared; + } data; + quintptr is_shared : 1; + quintptr is_null : 1; + quintptr packedType : sizeof(QMetaType) * 8 - 2; + + Private() noexcept : is_shared(false), is_null(true), packedType(0) {} explicit Private(const QMetaType &type) noexcept : is_shared(false), is_null(false) { if (type.d_ptr) @@ -468,45 +482,19 @@ class Q_CORE_EXPORT QVariant } Q_CORE_EXPORT ~Private(); - union Data - { - void *threeptr[3] = { nullptr, nullptr, nullptr }; - char c; - uchar uc; - short s; - signed char sc; - ushort us; - int i; - uint u; - long l; - ulong ul; - bool b; - double d; - float f; - qreal real; - qlonglong ll; - qulonglong ull; - QObject *o; - void *ptr; - PrivateShared *shared; - } data; - quintptr packedType : sizeof(QMetaType) * 8 - 2; - quintptr is_shared : 1; - quintptr is_null : 1; - - template - static constexpr bool CanUseInternalSpace = sizeof(T) <= sizeof(QVariant::Private::Data); - const void *storage() const - { return is_shared ? data.shared->data() : &data; } + { return is_shared ? data.shared->data() : &data.data; } const void *internalStorage() const - { Q_ASSERT(is_shared); return &data; } + { Q_ASSERT(is_shared); return &data.data; } // determine internal storage at compile time template const T &get() const - { return *static_cast(CanUseInternalSpace ? &data : data.shared->data()); } + { return *static_cast(storage()); } + template + void set(const T &t) + { *static_cast(CanUseInternalSpace ? &data.data : data.shared->data()) = t; } inline QMetaType type() const { @@ -750,7 +738,7 @@ namespace QtPrivate { static T object(const QVariant &v) { return qobject_cast(QMetaType::typeFlags(v.userType()) & QMetaType::PointerToQObject - ? v.d.data.o + ? v.d.get() : QVariantValueHelper::metaType(v)); } #endif diff --git a/tests/benchmarks/corelib/kernel/qvariant/tst_qvariant.cpp b/tests/benchmarks/corelib/kernel/qvariant/tst_qvariant.cpp index d3c779b3aa..be77cf5d6c 100644 --- a/tests/benchmarks/corelib/kernel/qvariant/tst_qvariant.cpp +++ b/tests/benchmarks/corelib/kernel/qvariant/tst_qvariant.cpp @@ -91,7 +91,7 @@ struct BigClass { double n,i,e,r,o,b; }; -static_assert(sizeof(BigClass) > sizeof(QVariant::Private::Data)); +static_assert(sizeof(BigClass) > sizeof(QVariant::Private::MaxInternalSize)); QT_BEGIN_NAMESPACE Q_DECLARE_TYPEINFO(BigClass, Q_MOVABLE_TYPE); QT_END_NAMESPACE @@ -101,7 +101,7 @@ struct SmallClass { char s; }; -static_assert(sizeof(SmallClass) <= sizeof(QVariant::Private::Data)); +static_assert(sizeof(SmallClass) <= sizeof(QVariant::Private::MaxInternalSize)); QT_BEGIN_NAMESPACE Q_DECLARE_TYPEINFO(SmallClass, Q_MOVABLE_TYPE); QT_END_NAMESPACE