From bd64f9397ac6c7aa4368d92138929e858e3df107 Mon Sep 17 00:00:00 2001 From: Lars Knoll Date: Wed, 8 Jul 2020 16:14:21 +0200 Subject: [PATCH] Refactor Q*Iterable Refactor the methods retrieving data in Q*Iterable so that we don't return pointers with unclear ownership. Instead, copy the data into a out pointer provided by the caller. This also means there is no need for the metatype flags anymore and we can remove those. Change-Id: I517de23a8ccfd608585ca00403aca0df2955f14b Reviewed-by: Fabian Kosmale --- src/corelib/kernel/qmetatype.cpp | 5 - src/corelib/kernel/qmetatype.h | 152 ++++++------------ src/corelib/kernel/qvariant.cpp | 54 ++++--- src/corelib/kernel/qvariant.h | 21 ++- src/corelib/kernel/qvariant_p.h | 3 +- .../corelib/kernel/qvariant/tst_qvariant.cpp | 19 +-- 6 files changed, 102 insertions(+), 152 deletions(-) diff --git a/src/corelib/kernel/qmetatype.cpp b/src/corelib/kernel/qmetatype.cpp index 9ccb0c5e75..d732285f08 100644 --- a/src/corelib/kernel/qmetatype.cpp +++ b/src/corelib/kernel/qmetatype.cpp @@ -1602,11 +1602,6 @@ static QtPrivate::QMetaTypeInterface *interfaceForType(int typeId) */ QMetaType::QMetaType(int typeId) : QMetaType(interfaceForType(typeId)) {} -namespace QtMetaTypePrivate { -const bool VectorBoolElements::true_element = true; -const bool VectorBoolElements::false_element = false; -} - namespace QtPrivate { #ifndef QT_BOOTSTRAPPED // Explicit instantiation definition diff --git a/src/corelib/kernel/qmetatype.h b/src/corelib/kernel/qmetatype.h index 5e2a83cf51..ed4ff87f92 100644 --- a/src/corelib/kernel/qmetatype.h +++ b/src/corelib/kernel/qmetatype.h @@ -652,25 +652,6 @@ ConverterFunctor::~ConverterFunctor() namespace QtMetaTypePrivate { -struct VariantData -{ - VariantData(QMetaType metaType_, - const void *data_, - const uint flags_) - : metaType(std::move(metaType_)) - , data(data_) - , flags(flags_) - { - } - VariantData(const VariantData &other) = delete; - const QMetaType metaType; - const void *data; - const uint flags; -private: - // copy constructor allowed to be implicit to silence level 4 warning from MSVC - VariantData &operator=(const VariantData &) = delete; -}; - template struct IteratorOwnerCommon { @@ -703,36 +684,17 @@ struct IteratorOwnerCommon template struct IteratorOwner : IteratorOwnerCommon { - static const void *getData(void * const *iterator) + static void getData(void * const *iterator, void *dataPtr) { - return &**static_cast(*iterator); + const_iterator *it = static_cast(*iterator); + typename const_iterator::value_type *data = static_cast(dataPtr); + *data = **it; } - static const void *getData(const_iterator it) + static void getData(const_iterator it, void *dataPtr) { - return &*it; - } -}; - -struct Q_CORE_EXPORT VectorBoolElements -{ - static const bool true_element; - static const bool false_element; -}; - -template<> -struct IteratorOwner::const_iterator> : IteratorOwnerCommon::const_iterator> -{ -public: - static const void *getData(void * const *iterator) - { - return **static_cast::const_iterator*>(*iterator) ? - &VectorBoolElements::true_element : &VectorBoolElements::false_element; - } - - static const void *getData(const std::vector::const_iterator& it) - { - return *it ? &VectorBoolElements::true_element : &VectorBoolElements::false_element; + typename const_iterator::value_type *data = static_cast(dataPtr); + *data = *it; } }; @@ -766,19 +728,19 @@ public: { } - static const void *getData(void * const *iterator) + static void getData(void * const *iterator, void *dataPtr) { - return *iterator; + *static_cast(dataPtr) = *static_cast(*iterator); } - static const void *getData(const value_type_OR_Dummy *it) + static void getData(const value_type_OR_Dummy *it, void *dataPtr) { - return it; + *static_cast(dataPtr) = *static_cast(it); } static bool equal(void * const *it, void * const *other) { - return static_cast(*it) == static_cast(*other); + return static_cast(*it) == static_cast(*other); } }; @@ -868,7 +830,6 @@ public: const void * _iterable; void *_iterator; QMetaType _metaType; - uint _metaType_flags; uint _iteratorCapabilities; // Iterator capabilities looks actually like // uint _iteratorCapabilities:4; @@ -876,11 +837,11 @@ public: // uint _containerCapabilities:4; // uint _unused:21; typedef int(*sizeFunc)(const void *p); - typedef const void * (*atFunc)(const void *p, int); + typedef void (*atFunc)(const void *p, int, void *); enum Position { ToBegin, ToEnd }; typedef void (*moveIteratorFunc)(const void *p, void **, Position position); typedef void (*advanceFunc)(void **p, int); - typedef VariantData (*getFunc)( void * const *p, const QMetaType &metaType, uint flags); + typedef void (*getFunc)( void * const *p, void *dataPtr); typedef void (*destroyIterFunc)(void **p); typedef bool (*equalIterFunc)(void * const *p, void * const *other); typedef void (*copyIterFunc)(void **, void * const *); @@ -905,47 +866,34 @@ public: { return ContainerAPI::size(static_cast(p)); } template - static const void* atImpl(const void *p, int idx) + static void atImpl(const void *p, int idx, void *dataPtr) { typename T::const_iterator i = static_cast(p)->begin(); std::advance(i, idx); - return IteratorOwner::getData(i); + IteratorOwner::getData(i, dataPtr); } - template - static void moveToBeginImpl(const void *container, void **iterator) - { IteratorOwner::assign(iterator, static_cast(container)->begin()); } - - template - static void moveToEndImpl(const void *container, void **iterator) - { IteratorOwner::assign(iterator, static_cast(container)->end()); } - template static void moveToImpl(const void *container, void **iterator, Position position) { - if (position == ToBegin) - moveToBeginImpl(container, iterator); - else - moveToEndImpl(container, iterator); + auto it = (position == ToBegin) ? + static_cast(container)->begin() : + static_cast(container)->end(); + IteratorOwner::assign(iterator, it); } - template - static VariantData getImpl(void * const *iterator, const QMetaType &metaType, uint flags) - { return VariantData(metaType, IteratorOwner::getData(iterator), flags); } - public: template QSequentialIterableImpl(const T*p) : _iterable(p) , _iterator(nullptr) , _metaType(QMetaType::fromType()) - , _metaType_flags(QTypeInfo::isPointer) , _iteratorCapabilities(ContainerAPI::IteratorCapabilities | (0 << 4) | (ContainerCapabilitiesImpl::ContainerCapabilities << (4+3))) , _size(sizeImpl) , _at(atImpl) , _moveTo(moveToImpl) , _append(ContainerCapabilitiesImpl::appendImpl) , _advance(IteratorOwner::advance) - , _get(getImpl) + , _get(IteratorOwner::getData) , _destroyIter(IteratorOwner::destroy) , _equalIter(IteratorOwner::equal) , _copyIter(IteratorOwner::assign) @@ -955,7 +903,6 @@ public: QSequentialIterableImpl() : _iterable(nullptr) , _iterator(nullptr) - , _metaType_flags(0) , _iteratorCapabilities(0 | (0 << 4) ) // no iterator capabilities, revision 0 , _size(nullptr) , _at(nullptr) @@ -987,10 +934,10 @@ public: _append(_iterable, newElement); } - inline VariantData getCurrent() const { return _get(&_iterator, _metaType, _metaType_flags); } + inline void getCurrent(void *dataPtr) const { _get(&_iterator, dataPtr); } - VariantData at(int idx) const - { return VariantData(_metaType, _at(_iterable, idx), _metaType_flags); } + void at(int idx, void *dataPtr) const + { return _at(_iterable, idx, dataPtr); } int size() const { Q_ASSERT(_iterable); return _size(_iterable); } @@ -1058,13 +1005,11 @@ public: void *_iterator; QMetaType _metaType_key; QMetaType _metaType_value; - uint _metaType_flags_key; - uint _metaType_flags_value; typedef int(*sizeFunc)(const void *p); typedef void (*findFunc)(const void *container, const void *p, void **iterator); typedef void (*beginFunc)(const void *p, void **); typedef void (*advanceFunc)(void **p, int); - typedef VariantData (*getFunc)(void * const *p, const QMetaType &metaTypeId, uint flags); + typedef void (*getFunc)(void * const *p, void *dataPtr); typedef void (*destroyIterFunc)(void **p); typedef bool (*equalIterFunc)(void * const *p, void * const *other); typedef void (*copyIterFunc)(void **, void * const *); @@ -1103,12 +1048,18 @@ public: { IteratorOwner::assign(iterator, static_cast(container)->end()); } template - static VariantData getKeyImpl(void * const *iterator, const QMetaType &metaType, uint flags) - { return VariantData(metaType, &AssociativeContainerAccessor::getKey(*static_cast(*iterator)), flags); } + static void getKeyImpl(void * const *iterator, void *dataPtr) + { + auto *data = static_cast(dataPtr); + *data = AssociativeContainerAccessor::getKey(*static_cast(*iterator)); + } template - static VariantData getValueImpl(void * const *iterator, const QMetaType &metaType, uint flags) - { return VariantData(metaType, &AssociativeContainerAccessor::getValue(*static_cast(*iterator)), flags); } + static void getValueImpl(void * const *iterator, void *dataPtr) + { + auto *data = static_cast(dataPtr); + *data = AssociativeContainerAccessor::getValue(*static_cast(*iterator)); + } public: template QAssociativeIterableImpl(const T*p) @@ -1116,8 +1067,6 @@ public: , _iterator(nullptr) , _metaType_key(QMetaType::fromType()) , _metaType_value(QMetaType::fromType()) - , _metaType_flags_key(QTypeInfo::isPointer) - , _metaType_flags_value(QTypeInfo::isPointer) , _size(sizeImpl) , _find(findImpl) , _begin(beginImpl) @@ -1134,8 +1083,6 @@ public: QAssociativeIterableImpl() : _iterable(nullptr) , _iterator(nullptr) - , _metaType_flags_key(0) - , _metaType_flags_value(0) , _size(nullptr) , _find(nullptr) , _begin(nullptr) @@ -1156,11 +1103,11 @@ public: inline void destroyIter() { _destroyIter(&_iterator); } - inline VariantData getCurrentKey() const { return _getKey(&_iterator, _metaType_key, _metaType_flags_key); } - inline VariantData getCurrentValue() const { return _getValue(&_iterator, _metaType_value, _metaType_flags_value); } + inline void getCurrentKey(void *dataPtr) const { return _getKey(&_iterator, dataPtr); } + inline void getCurrentValue(void *dataPtr) const { return _getValue(&_iterator, dataPtr); } - inline void find(const VariantData &key) - { _find(_iterable, key.data, &_iterator); } + inline void find(const void *key) + { _find(_iterable, key, &_iterator); } int size() const { Q_ASSERT(_iterable); return _size(_iterable); } @@ -1183,31 +1130,28 @@ struct QAssociativeIterableConvertFunctor class QPairVariantInterfaceImpl { +public: const void *_pair; QMetaType _metaType_first; QMetaType _metaType_second; - uint _metaType_flags_first; - uint _metaType_flags_second; - typedef VariantData (*getFunc)(const void * const *p, const QMetaType &metaType, uint flags); + typedef void (*getFunc)(const void * const *p, void *); getFunc _getFirst; getFunc _getSecond; template - static VariantData getFirstImpl(const void * const *pair, const QMetaType &metaType, uint flags) - { return VariantData(metaType, &static_cast(*pair)->first, flags); } + static void getFirstImpl(const void * const *pair, void *dataPtr) + { *static_cast(dataPtr) = static_cast(*pair)->first; } template - static VariantData getSecondImpl(const void * const *pair, const QMetaType &metaType, uint flags) - { return VariantData(metaType, &static_cast(*pair)->second, flags); } + static void getSecondImpl(const void * const *pair, void *dataPtr) + { *static_cast(dataPtr) = static_cast(*pair)->second; } public: template QPairVariantInterfaceImpl(const T*p) : _pair(p) , _metaType_first(QMetaType::fromType()) , _metaType_second(QMetaType::fromType()) - , _metaType_flags_first(QTypeInfo::isPointer) - , _metaType_flags_second(QTypeInfo::isPointer) , _getFirst(getFirstImpl) , _getSecond(getSecondImpl) { @@ -1215,15 +1159,13 @@ public: QPairVariantInterfaceImpl() : _pair(nullptr) - , _metaType_flags_first(0) - , _metaType_flags_second(0) , _getFirst(nullptr) , _getSecond(nullptr) { } - inline VariantData first() const { return _getFirst(&_pair, _metaType_first, _metaType_flags_first); } - inline VariantData second() const { return _getSecond(&_pair, _metaType_second, _metaType_flags_second); } + inline void first(void *dataPtr) const { _getFirst(&_pair, dataPtr); } + inline void second(void *dataPtr) const { _getSecond(&_pair, dataPtr); } }; QT_METATYPE_PRIVATE_DECLARE_TYPEINFO(QPairVariantInterfaceImpl, Q_MOVABLE_TYPE) diff --git a/src/corelib/kernel/qvariant.cpp b/src/corelib/kernel/qvariant.cpp index 741a477860..c28476af17 100644 --- a/src/corelib/kernel/qvariant.cpp +++ b/src/corelib/kernel/qvariant.cpp @@ -4243,24 +4243,19 @@ QSequentialIterable::const_iterator QSequentialIterable::end() const return it; } -static const QVariant variantFromVariantDataHelper(const QtMetaTypePrivate::VariantData &d) { - QVariant v; - if (d.metaType == QMetaType::fromType()) - v = *reinterpret_cast(d.data); - else - v = QVariant(d.metaType, d.data); - if (d.flags & QVariantConstructionFlags::ShouldDeleteVariantData) - d.metaType.destroy(const_cast(d.data)); - return v; -} - /*! Returns the element at position \a idx in the container. */ QVariant QSequentialIterable::at(int idx) const { - const QtMetaTypePrivate::VariantData d = m_impl.at(idx); - return variantFromVariantDataHelper(d); + QVariant v(m_impl._metaType); + void *dataPtr; + if (m_impl._metaType == QMetaType::fromType()) + dataPtr = &v; + else + dataPtr = v.data(); + m_impl.at(idx, dataPtr); + return v; } /*! @@ -4336,8 +4331,14 @@ QSequentialIterable::const_iterator::operator=(const const_iterator &other) */ const QVariant QSequentialIterable::const_iterator::operator*() const { - const QtMetaTypePrivate::VariantData d = m_impl.getCurrent(); - return variantFromVariantDataHelper(d); + QVariant v(m_impl._metaType); + void *dataPtr; + if (m_impl._metaType == QMetaType::fromType()) + dataPtr = &v; + else + dataPtr = v.data(); + m_impl.getCurrent(dataPtr); + return v; } /*! @@ -4534,8 +4535,7 @@ void QAssociativeIterable::const_iterator::end() void QAssociativeIterable::const_iterator::find(const QVariant &key) { Q_ASSERT(key.metaType() == m_impl._metaType_key); - const QtMetaTypePrivate::VariantData dkey(key.metaType(), key.constData(), 0 /*key.flags()*/); - m_impl.find(dkey); + m_impl.find(key.constData()); } /*! @@ -4664,8 +4664,14 @@ QAssociativeIterable::const_iterator::operator=(const const_iterator &other) */ const QVariant QAssociativeIterable::const_iterator::operator*() const { - const QtMetaTypePrivate::VariantData d = m_impl.getCurrentValue(); - return variantFromVariantDataHelper(d); + QVariant v(m_impl._metaType_value); + void *dataPtr; + if (m_impl._metaType_value == QMetaType::fromType()) + dataPtr = &v; + else + dataPtr = v.data(); + m_impl.getCurrentValue(dataPtr); + return v; } /*! @@ -4673,8 +4679,14 @@ const QVariant QAssociativeIterable::const_iterator::operator*() const */ const QVariant QAssociativeIterable::const_iterator::key() const { - const QtMetaTypePrivate::VariantData d = m_impl.getCurrentKey(); - return variantFromVariantDataHelper(d); + QVariant v(m_impl._metaType_key); + void *dataPtr; + if (m_impl._metaType_key == QMetaType::fromType()) + dataPtr = &v; + else + dataPtr = v.data(); + m_impl.getCurrentKey(dataPtr); + return v; } /*! diff --git a/src/corelib/kernel/qvariant.h b/src/corelib/kernel/qvariant.h index 281080d332..b738b80d38 100644 --- a/src/corelib/kernel/qvariant.h +++ b/src/corelib/kernel/qvariant.h @@ -816,15 +816,20 @@ namespace QtPrivate { if (QMetaType::hasRegisteredConverterFunction(typeId, qMetaTypeId()) && !(typeId == qMetaTypeId >())) { QtMetaTypePrivate::QPairVariantInterfaceImpl pi = v.value(); - const QtMetaTypePrivate::VariantData d1 = pi.first(); - QVariant v1(d1.metaType, d1.data); - if (d1.metaType == QMetaType::fromType()) - v1 = *reinterpret_cast(d1.data); + QVariant v1(pi._metaType_first); + void *dataPtr; + if (pi._metaType_first == QMetaType::fromType()) + dataPtr = &v1; + else + dataPtr = v1.data(); + pi.first(dataPtr); - const QtMetaTypePrivate::VariantData d2 = pi.second(); - QVariant v2(d2.metaType, d2.data); - if (d2.metaType == QMetaType::fromType()) - v2 = *reinterpret_cast(d2.data); + QVariant v2(pi._metaType_second); + if (pi._metaType_second == QMetaType::fromType()) + dataPtr = &v2; + else + dataPtr = v2.data(); + pi.second(dataPtr); return QPair(v1, v2); } diff --git a/src/corelib/kernel/qvariant_p.h b/src/corelib/kernel/qvariant_p.h index 95bd91fe1d..0682d1d6b3 100644 --- a/src/corelib/kernel/qvariant_p.h +++ b/src/corelib/kernel/qvariant_p.h @@ -106,8 +106,7 @@ inline T *v_cast(QVariant::Private *d, T * = nullptr) enum QVariantConstructionFlags : uint { Default = 0x0, - PointerType = 0x1, - ShouldDeleteVariantData = 0x2 // only used in Q*Iterable + PointerType = 0x1 }; //a simple template that avoids to allocate 2 memory chunks when creating a QVariant diff --git a/tests/auto/corelib/kernel/qvariant/tst_qvariant.cpp b/tests/auto/corelib/kernel/qvariant/tst_qvariant.cpp index b1cf955e93..e18b6e024a 100644 --- a/tests/auto/corelib/kernel/qvariant/tst_qvariant.cpp +++ b/tests/auto/corelib/kernel/qvariant/tst_qvariant.cpp @@ -4512,7 +4512,6 @@ void tst_QVariant::shouldDeleteVariantDataWorksForSequential() iterator._iteratorCapabilities = QtMetaTypePrivate::RandomAccessCapability | QtMetaTypePrivate::BiDirectionalCapability | QtMetaTypePrivate::ForwardCapability; - iterator._metaType_flags = QVariantConstructionFlags::ShouldDeleteVariantData; iterator._size = [](const void *) {return 1;}; iterator._metaType = QMetaType::fromType(); @@ -4522,13 +4521,13 @@ void tst_QVariant::shouldDeleteVariantDataWorksForSequential() iterator._destroyIter = [](void **){}; iterator._equalIter = [](void * const *, void * const *){return true; /*all iterators are nullptr*/}; iterator._destroyIter = [](void **){}; - iterator._at = [](const void *, int ) -> void const * { + iterator._at = [](const void *, int, void *dataPtr) -> void { MyType mytype {1, "eins"}; - return QMetaType::create(qMetaTypeId(), &mytype); + *static_cast(dataPtr) = mytype; }; - iterator._get = [](void * const *, const QMetaType &, uint) -> QtMetaTypePrivate::VariantData { + iterator._get = [](void * const *, void *dataPtr) -> void { MyType mytype {2, "zwei"}; - return {QMetaType::fromType(), QMetaType::create(qMetaTypeId(), &mytype), QVariantConstructionFlags::ShouldDeleteVariantData}; + *static_cast(dataPtr) = mytype; }; QSequentialIterable iterable {iterator}; QVariant value1 = iterable.at(0); @@ -4546,8 +4545,6 @@ void tst_QVariant::shouldDeleteVariantDataWorksForAssociative() QCOMPARE(instanceCount, 0); { QtMetaTypePrivate::QAssociativeIterableImpl iterator {}; - iterator._metaType_flags_key = QVariantConstructionFlags::ShouldDeleteVariantData; - iterator._metaType_flags_value = QVariantConstructionFlags::ShouldDeleteVariantData; iterator._size = [](const void *) {return 1;}; iterator._metaType_value = QMetaType::fromType(); @@ -4561,21 +4558,21 @@ void tst_QVariant::shouldDeleteVariantDataWorksForAssociative() iterator._find = [](const void *, const void *, void **iterator ) -> void { (*iterator) = reinterpret_cast(quintptr(42)); }; - iterator._getKey = [](void * const *iterator, const QMetaType &, uint) -> QtMetaTypePrivate::VariantData { + iterator._getKey = [](void * const *iterator, void *dataPtr) -> void { MyType mytype {1, "key"}; if (reinterpret_cast(*iterator) == 42) { mytype.number = 42; mytype.text = "find_key"; } - return {QMetaType::fromType(), QMetaType::create(qMetaTypeId(), &mytype), QVariantConstructionFlags::ShouldDeleteVariantData}; + *static_cast(dataPtr) = mytype; }; - iterator._getValue = [](void * const *iterator, const QMetaType &, uint) -> QtMetaTypePrivate::VariantData { + iterator._getValue = [](void * const *iterator, void *dataPtr) -> void { MyType mytype {2, "value"}; if (reinterpret_cast(*iterator) == 42) { mytype.number = 42; mytype.text = "find_value"; } - return {QMetaType::fromType(), QMetaType::create(qMetaTypeId(), &mytype), QVariantConstructionFlags::ShouldDeleteVariantData}; + *static_cast(dataPtr) = mytype; }; QAssociativeIterable iterable {iterator}; auto it = iterable.begin();