From 04641454beb27f667062dbf79116729f159b0041 Mon Sep 17 00:00:00 2001 From: Lars Knoll Date: Mon, 28 Sep 2020 10:32:00 +0200 Subject: Disable moving of QProperty The semantics are not very intuitive, and it opens a can of worms with regards to what should happen with observers that observe that property. Change-Id: I6fb00b7693904b968224cc87d098bbd0ea776ba3 Reviewed-by: Ulf Hermann --- src/corelib/kernel/qproperty.cpp | 47 ++------ src/corelib/kernel/qproperty.h | 10 +- src/corelib/kernel/qproperty_p.h | 12 ++ src/corelib/kernel/qpropertyprivate.h | 4 +- .../corelib/kernel/qproperty/tst_qproperty.cpp | 132 ++++++--------------- 5 files changed, 54 insertions(+), 151 deletions(-) diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp index 4fdf0d8100..3a2c8dcc0c 100644 --- a/src/corelib/kernel/qproperty.cpp +++ b/src/corelib/kernel/qproperty.cpp @@ -196,39 +196,6 @@ QMetaType QUntypedPropertyBinding::valueMetaType() const return d->valueMetaType(); } -QPropertyBindingData::QPropertyBindingData(QPropertyBindingData &&other, QUntypedPropertyData *propertyDataPtr) -{ - std::swap(d_ptr, other.d_ptr); - QPropertyBindingDataPointer d{this}; - d.setFirstObserver(nullptr); - if (auto binding = d.bindingPtr()) - binding->setProperty(propertyDataPtr); -} - -void QPropertyBindingData::moveAssign(QPropertyBindingData &&other, QUntypedPropertyData *propertyDataPtr) -{ - if (&other == this) - return; - - QPropertyBindingDataPointer d{this}; - auto observer = d.firstObserver(); - d.setFirstObserver(nullptr); - - if (auto binding = d.bindingPtr()) { - binding->unlinkAndDeref(); - d_ptr &= FlagMask; - } - - std::swap(d_ptr, other.d_ptr); - - if (auto binding = d.bindingPtr()) - binding->setProperty(propertyDataPtr); - - d.setFirstObserver(observer.ptr); - - // The caller will have to notify observers. -} - QPropertyBindingData::~QPropertyBindingData() { QPropertyBindingDataPointer d{this}; @@ -286,6 +253,12 @@ QUntypedPropertyBinding QPropertyBindingData::setBinding(const QUntypedPropertyB return QUntypedPropertyBinding(oldBinding.data()); } +QPropertyBindingData::QPropertyBindingData(QPropertyBindingData &&other) : d_ptr(std::exchange(other.d_ptr, 0)) +{ + QPropertyBindingDataPointer d{this}; + d.fixupFirstObserverAfterMove(); +} + QPropertyBindingPrivate *QPropertyBindingData::binding() const { QPropertyBindingDataPointer d{this}; @@ -693,12 +666,6 @@ QString QPropertyBindingError::description() const \a other was pointing to. */ -/*! - \fn template QProperty &QProperty::operator=(QProperty &&other) - - Move-assigns \a other to this QProperty instance. -*/ - /*! \fn template QProperty::QProperty(const QPropertyBinding &binding) @@ -1395,7 +1362,7 @@ struct QBindingStoragePrivate if (index == newData->size) index = 0; } - new (pp + index) Pair{p->data, QPropertyBindingData(std::move(p->bindingData), p->data)}; + new (pp + index) Pair{p->data, QPropertyBindingData(std::move(p->bindingData))}; } } // data has been moved, no need to call destructors on old Pairs diff --git a/src/corelib/kernel/qproperty.h b/src/corelib/kernel/qproperty.h index 09b2642e73..156696dace 100644 --- a/src/corelib/kernel/qproperty.h +++ b/src/corelib/kernel/qproperty.h @@ -315,7 +315,6 @@ public: QProperty() = default; explicit QProperty(parameter_type initialValue) : QPropertyData(initialValue) {} explicit QProperty(rvalue_ref initialValue) : QPropertyData(std::move(initialValue)) {} - QProperty(QProperty &&other) : QPropertyData(std::move(other.val)), d(std::move(other.d), this) { notify(); } explicit QProperty(const QPropertyBinding &binding) : QProperty() { setBinding(binding); } @@ -329,13 +328,6 @@ public: template explicit QProperty(Functor &&f); #endif - QProperty &operator=(QProperty &&other) - { - this->val = std::move(other.val); - d.moveAssign(std::move(other.d), this); - notify(); - return *this; - } ~QProperty() = default; parameter_type value() const @@ -460,7 +452,7 @@ private: d.notifyObservers(this); } - Q_DISABLE_COPY(QProperty) + Q_DISABLE_COPY_MOVE(QProperty) }; namespace Qt { diff --git a/src/corelib/kernel/qproperty_p.h b/src/corelib/kernel/qproperty_p.h index 614756a670..1bf308e8ea 100644 --- a/src/corelib/kernel/qproperty_p.h +++ b/src/corelib/kernel/qproperty_p.h @@ -81,6 +81,7 @@ struct Q_AUTOTEST_EXPORT QPropertyBindingDataPointer observer->prev = reinterpret_cast(&(ptr->d_ptr)); ptr->d_ptr = (reinterpret_cast(observer) & ~QtPrivate::QPropertyBindingData::FlagMask); } + void fixupFirstObserverAfterMove() const; void addObserver(QPropertyObserver *observer); void setFirstObserver(QPropertyObserver *observer); QPropertyObserverPointer firstObserver() const; @@ -293,6 +294,17 @@ inline void QPropertyBindingDataPointer::setFirstObserver(QPropertyObserver *obs ptr->d_ptr = reinterpret_cast(observer) | (ptr->d_ptr & QtPrivate::QPropertyBindingData::FlagMask); } +inline void QPropertyBindingDataPointer::fixupFirstObserverAfterMove() const +{ + // If QPropertyBindingData has been moved, and it has an observer + // we have to adjust the firstObesrver's prev pointer to point to + // the moved to QPropertyBindingData's d_ptr + if (ptr->d_ptr & QtPrivate::QPropertyBindingData::BindingBit) + return; // nothing to do if the observer is stored in the binding + if (auto observer = firstObserver()) + observer.ptr->prev = reinterpret_cast(&(ptr->d_ptr)); +} + inline QPropertyObserverPointer QPropertyBindingDataPointer::firstObserver() const { if (auto *binding = bindingPtr()) diff --git a/src/corelib/kernel/qpropertyprivate.h b/src/corelib/kernel/qpropertyprivate.h index dd344c209a..26877e0315 100644 --- a/src/corelib/kernel/qpropertyprivate.h +++ b/src/corelib/kernel/qpropertyprivate.h @@ -88,12 +88,10 @@ class Q_CORE_EXPORT QPropertyBindingData Q_DISABLE_COPY(QPropertyBindingData) public: QPropertyBindingData() = default; - QPropertyBindingData(QPropertyBindingData &&other, QUntypedPropertyData *propertyDataPtr); + QPropertyBindingData(QPropertyBindingData &&other); QPropertyBindingData &operator=(QPropertyBindingData &&other) = delete; ~QPropertyBindingData(); - void moveAssign(QPropertyBindingData &&other, QUntypedPropertyData *propertyDataPtr); - bool hasBinding() const { return d_ptr & BindingBit; } QUntypedPropertyBinding setBinding(const QUntypedPropertyBinding &newBinding, diff --git a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp index d71053ff04..565916b781 100644 --- a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp +++ b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp @@ -45,13 +45,9 @@ private slots: void bindingAfterUse(); void switchBinding(); void avoidDependencyAllocationAfterFirstEval(); - void propertyArrays(); void boolProperty(); void takeBinding(); void replaceBinding(); - void swap(); - void moveNotifies(); - void moveCtor(); void changeHandler(); void propertyChangeHandlerApi(); void subscribe(); @@ -238,27 +234,6 @@ void tst_QProperty::avoidDependencyAllocationAfterFirstEval() QCOMPARE(QPropertyBindingDataPointer::get(propWithBinding).bindingPtr()->dependencyObserverCount, 2u); } -void tst_QProperty::propertyArrays() -{ - std::vector> properties; - - int expectedSum = 0; - for (int i = 0; i < 10; ++i) { - properties.emplace_back(i); - expectedSum += i; - } - - QProperty sum([&]() { - return std::accumulate(properties.begin(), properties.end(), 0); - }); - - QCOMPARE(sum.value(), expectedSum); - - properties[4] = properties[4] + 42; - expectedSum += 42; - QCOMPARE(sum.value(), expectedSum); -} - void tst_QProperty::boolProperty() { QProperty first(true); @@ -311,74 +286,6 @@ void tst_QProperty::replaceBinding() QCOMPARE(second.value(), 100); } -void tst_QProperty::swap() -{ - QProperty firstDependency(1); - QProperty secondDependency(2); - - QProperty first(Qt::makePropertyBinding(firstDependency)); - QProperty second(Qt::makePropertyBinding(secondDependency)); - - QCOMPARE(first.value(), 1); - QCOMPARE(second.value(), 2); - - std::swap(first, second); - - QCOMPARE(first.value(), 2); - QCOMPARE(second.value(), 1); - - secondDependency = 20; - QCOMPARE(first.value(), 20); - QCOMPARE(second.value(), 1); - - firstDependency = 100; - QCOMPARE(first.value(), 20); - QCOMPARE(second.value(), 100); -} - -void tst_QProperty::moveNotifies() -{ - QProperty first(1); - QProperty second(2); - - QProperty propertyInTheMiddle(Qt::makePropertyBinding(first)); - - QProperty finalProp1(Qt::makePropertyBinding(propertyInTheMiddle)); - QProperty finalProp2(Qt::makePropertyBinding(propertyInTheMiddle)); - - QCOMPARE(finalProp1.value(), 1); - QCOMPARE(finalProp2.value(), 1); - - QCOMPARE(QPropertyBindingDataPointer::get(propertyInTheMiddle).observerCount(), 2); - - QProperty other(Qt::makePropertyBinding(second)); - QCOMPARE(other.value(), 2); - - QProperty otherDep(Qt::makePropertyBinding(other)); - QCOMPARE(otherDep.value(), 2); - QCOMPARE(QPropertyBindingDataPointer::get(other).observerCount(), 1); - - propertyInTheMiddle = std::move(other); - - QCOMPARE(QPropertyBindingDataPointer::get(other).observerCount(), 0); - - QCOMPARE(finalProp1.value(), 2); - QCOMPARE(finalProp2.value(), 2); -} - -void tst_QProperty::moveCtor() -{ - QProperty first(1); - - QProperty intermediate(Qt::makePropertyBinding(first)); - QCOMPARE(intermediate.value(), 1); - QCOMPARE(QPropertyBindingDataPointer::get(first).observerCount(), 1); - - QProperty targetProp(std::move(first)); - - QCOMPARE(QPropertyBindingDataPointer::get(targetProp).observerCount(), 0); -} - void tst_QProperty::changeHandler() { QProperty testProperty(0); @@ -570,6 +477,11 @@ void tst_QProperty::bindingLoop() class ReallocTester : public QObject { + /* + * This class and the realloc test rely on the fact that the internal property hashmap has an + * initial capacity of 8 and a load factor of 0.5. Thus, it is necessary to cause actions which + * allocate 5 different QPropertyBindingData + * */ Q_OBJECT Q_PROPERTY(int prop1 READ prop1 WRITE setProp1 BINDABLE bindableProp1) Q_PROPERTY(int prop2 READ prop2 WRITE setProp2 BINDABLE bindableProp2) @@ -595,12 +507,34 @@ public: void tst_QProperty::realloc() { - ReallocTester tester; - tester.bindableProp1().setBinding([&](){return tester.prop5();}); - tester.bindableProp2().setBinding([&](){return tester.prop5();}); - tester.bindableProp3().setBinding([&](){return tester.prop5();}); - tester.bindableProp4().setBinding([&](){return tester.prop5();}); - tester.bindableProp5().setBinding([&]() -> int{return 42;}); + { + // Triggering a reallocation does not crash + ReallocTester tester; + tester.bindableProp1().setBinding([&](){return tester.prop5();}); + tester.bindableProp2().setBinding([&](){return tester.prop5();}); + tester.bindableProp3().setBinding([&](){return tester.prop5();}); + tester.bindableProp4().setBinding([&](){return tester.prop5();}); + tester.bindableProp5().setBinding([&]() -> int{return 42;}); + QCOMPARE(tester.prop1(), 42); + } + { + // After a reallocation, property observers still work + ReallocTester tester; + int modificationCount = 0; + QPropertyChangeHandler observer {[&](){ ++modificationCount; }}; + tester.bindableProp1().observe(&observer); + tester.setProp1(12); + QCOMPARE(modificationCount, 1); + QCOMPARE(tester.prop1(), 12); + + tester.bindableProp1().setBinding([&](){return tester.prop5();}); + tester.bindableProp2().setBinding([&](){return tester.prop5();}); + tester.bindableProp3().setBinding([&](){return tester.prop5();}); + tester.bindableProp4().setBinding([&](){return tester.prop5();}); + tester.bindableProp5().setBinding([&]() -> int{return 42;}); + QEXPECT_FAIL("", "ChangeHandler bug with eager properties", Continue); + QCOMPARE(modificationCount, 2); + } }; void tst_QProperty::changePropertyFromWithinChangeHandler() -- cgit v1.2.3