From e4d62651c278c468f71ce097979bc1183ffd0713 Mon Sep 17 00:00:00 2001 From: Fabian Kosmale Date: Wed, 18 Aug 2021 14:10:12 +0200 Subject: [PATCH] Re-add QPropertyAlias functionality As QPropertyAlias was public by accident in 6.0, we have to ensure that it still works in 6.2. This re-adds some tests for it, and reimplements the unlinking functionality. To avoid performance regressions in hot-paths, a new unlink_fast function is added, which behaves like the old unlink: It ignores the special handling for QPropertyAlias, so that we can skip the tag check. It is only used in QPropertyObserverNodeProtector and clearDependencyObservers, where we already know the type of the observer. Fixes: QTBUG-95846 Pick-to: 6.2 Change-Id: Ifb405b8327c4d61c673b1a912ed6e169d27c2d8f Reviewed-by: Ulf Hermann --- src/corelib/kernel/qproperty.cpp | 43 +++++++++++++++---- src/corelib/kernel/qproperty.h | 6 ++- src/corelib/kernel/qproperty_p.h | 3 +- .../kernel/qproperty/tst_qproperty.cpp | 17 ++++++++ 4 files changed, 57 insertions(+), 12 deletions(-) diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp index 3123858d83..942abfe6ba 100644 --- a/src/corelib/kernel/qproperty.cpp +++ b/src/corelib/kernel/qproperty.cpp @@ -558,9 +558,10 @@ QPropertyObserver::QPropertyObserver(ChangeHandler changeHandler) d.setChangeHandler(changeHandler); } -QPropertyObserver::QPropertyObserver(QUntypedPropertyData *) +QPropertyObserver::QPropertyObserver(QUntypedPropertyData *data) { - // ### Qt 7: Remove, currently left for binary compatibility + aliasData = data; + next.setTag(ObserverIsAlias); } /*! \internal @@ -609,16 +610,38 @@ QPropertyObserver &QPropertyObserver::operator=(QPropertyObserver &&other) noexc return *this; } +#define UNLINK_COMMON \ + if (ptr->next) \ + ptr->next->prev = ptr->prev; \ + if (ptr->prev) \ + ptr->prev.setPointer(ptr->next.data()); \ + ptr->next = nullptr; \ + ptr->prev.clear(); + +/*! + \internal + Unlinks + */ void QPropertyObserverPointer::unlink() { - if (ptr->next) - ptr->next->prev = ptr->prev; - if (ptr->prev) - ptr->prev.setPointer(ptr->next.data()); - ptr->next = nullptr; - ptr->prev.clear(); + UNLINK_COMMON + if (ptr->next.tag() == QPropertyObserver::ObserverIsAlias) + ptr->aliasData = nullptr; } +/*! + \internal + Like unlink, but does not handle ObserverIsAlias. + Must only be called in places where we know that we are not dealing + with such an observer. + */ +void QPropertyObserverPointer::unlink_fast() +{ + Q_ASSERT(ptr->next.tag() != QPropertyObserver::ObserverIsAlias); + UNLINK_COMMON +} +#undef UNLINK_COMMON + void QPropertyObserverPointer::setChangeHandler(QPropertyObserver::ChangeHandler changeHandler) { Q_ASSERT(ptr->next.tag() != QPropertyObserver::ObserverIsPlaceholder); @@ -666,7 +689,7 @@ struct [[nodiscard]] QPropertyObserverNodeProtector { ~QPropertyObserverNodeProtector() { QPropertyObserverPointer d{static_cast(&m_placeHolder)}; - d.unlink(); + d.unlink_fast(); } }; @@ -721,6 +744,8 @@ void QPropertyObserverPointer::notify(QUntypedPropertyData *propertyDataPtr) case QPropertyObserver::ObserverIsPlaceholder: // recursion is already properly handled somewhere else break; + case QPropertyObserver::ObserverIsAlias: + break; default: Q_UNREACHABLE(); } observer = next; diff --git a/src/corelib/kernel/qproperty.h b/src/corelib/kernel/qproperty.h index 133c7a7e14..356eb43cd3 100644 --- a/src/corelib/kernel/qproperty.h +++ b/src/corelib/kernel/qproperty.h @@ -223,7 +223,8 @@ public: enum ObserverTag { ObserverNotifiesBinding, // observer was installed to notify bindings that obsverved property changed ObserverNotifiesChangeHandler, // observer is a change handler, which runs on every change - ObserverIsPlaceholder // the observer before this one is currently evaluated in QPropertyObserver::notifyObservers. + ObserverIsPlaceholder, // the observer before this one is currently evaluated in QPropertyObserver::notifyObservers. + ObserverIsAlias }; protected: using ChangeHandler = void (*)(QPropertyObserver*, QUntypedPropertyData *); @@ -244,6 +245,7 @@ private: union { QPropertyBindingPrivate *binding = nullptr; ChangeHandler changeHandler; + QUntypedPropertyData *aliasData; }; }; @@ -266,7 +268,7 @@ protected: QUntypedPropertyData *aliasedProperty() const { - return nullptr; + return aliasData; } private: diff --git a/src/corelib/kernel/qproperty_p.h b/src/corelib/kernel/qproperty_p.h index e77bc4d834..55f0dc032c 100644 --- a/src/corelib/kernel/qproperty_p.h +++ b/src/corelib/kernel/qproperty_p.h @@ -103,6 +103,7 @@ struct QPropertyObserverPointer QPropertyObserver *ptr = nullptr; void unlink(); + void unlink_fast(); void setBindingToNotify(QPropertyBindingPrivate *binding); void setBindingToNotify_unsafe(QPropertyBindingPrivate *binding); @@ -289,7 +290,7 @@ public: void clearDependencyObservers() { for (size_t i = 0; i < qMin(dependencyObserverCount, inlineDependencyObservers.size()); ++i) { QPropertyObserverPointer p{&inlineDependencyObservers[i]}; - p.unlink(); + p.unlink_fast(); } if (heapObservers) heapObservers->clear(); diff --git a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp index 6e63792a11..53c59d5170 100644 --- a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp +++ b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp @@ -115,6 +115,8 @@ private slots: void bindableInterfaceOfCompatPropertyUsesSetter(); void selfBindingShouldNotCrash(); + + void qpropertyAlias(); }; void tst_QProperty::functorBinding() @@ -1856,6 +1858,21 @@ void tst_QProperty::selfBindingShouldNotCrash() QVERIFY(i.binding().error().hasError()); } +void tst_QProperty::qpropertyAlias() +{ + std::unique_ptr> i {new QProperty}; + QPropertyAlias alias(i.get()); + QVERIFY(alias.isValid()); + alias.setValue(42); + QCOMPARE(i->value(), 42); + QProperty j; + i->setBinding([&]() -> int { return j; }); + j.setValue(42); + QCOMPARE(alias.value(), 42); + i.reset(); + QVERIFY(!alias.isValid()); +} + QTEST_MAIN(tst_QProperty); #undef QT_SOURCE_LOCATION_NAMESPACE