From be1ce6b26909256e62c2b39e4de9f8f79d070937 Mon Sep 17 00:00:00 2001 From: Lars Knoll Date: Wed, 8 Jul 2020 10:57:21 +0200 Subject: [PATCH] Separate the error case when evaluating bindings There's no point in returning a usually empty error when evaluating bindings, adding overhead to the regular code path. Instead, the error can be set on the currently evaluating binding if required. This streamlines the functor used to wrap the binding and should thus expand to less code and execute faster in the regular case. To achieve this, expose a pointer to the currently evaluating binding in the private API (as QtQml needs it to be able to report errors). The error case now requires one additional TLS lookup, but we don't really care about performance in that case anyway. Change-Id: Iecb450e765244930a41d813fcf8eb4013957a6a3 Reviewed-by: Fabian Kosmale --- src/corelib/kernel/qproperty.cpp | 5 +++ src/corelib/kernel/qproperty.h | 33 +++---------------- src/corelib/kernel/qpropertybinding.cpp | 13 +++----- src/corelib/kernel/qpropertybinding_p.h | 5 +++ .../kernel/qproperty/tst_qproperty.cpp | 14 ++++---- 5 files changed, 25 insertions(+), 45 deletions(-) diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp index 92c9afc6d85..f7039cc62ad 100644 --- a/src/corelib/kernel/qproperty.cpp +++ b/src/corelib/kernel/qproperty.cpp @@ -206,6 +206,11 @@ BindingEvaluationState::~BindingEvaluationState() *currentState = previousState; } +QPropertyBindingPrivate *QPropertyBindingPrivate::currentlyEvaluatingBinding() +{ + return currentBindingEvaluationState ? currentBindingEvaluationState->binding : nullptr; +} + void QPropertyBase::evaluateIfDirty() { QPropertyBasePointer d{this}; diff --git a/src/corelib/kernel/qproperty.h b/src/corelib/kernel/qproperty.h index 986c6afbc91..8a712d395e2 100644 --- a/src/corelib/kernel/qproperty.h +++ b/src/corelib/kernel/qproperty.h @@ -119,9 +119,8 @@ private: class Q_CORE_EXPORT QUntypedPropertyBinding { public: - using BindingEvaluationResult = QPropertyBindingError; // writes binding result into dataPtr - using BindingEvaluationFunction = std::function; + using BindingEvaluationFunction = std::function; QUntypedPropertyBinding(); QUntypedPropertyBinding(const QMetaType &metaType, BindingEvaluationFunction function, const QPropertyBindingSourceLocation &location); @@ -152,19 +151,10 @@ class QPropertyBinding : public QUntypedPropertyBinding struct BindingAdaptor { Functor impl; - QUntypedPropertyBinding::BindingEvaluationResult operator()(const QMetaType &/*metaType*/, void *dataPtr) + void operator()(const QMetaType &/*metaType*/, void *dataPtr) { - std::variant result(impl()); - if (auto errorPtr = std::get_if(&result)) - return *errorPtr; - - if (auto valuePtr = std::get_if(&result)) { - PropertyType *propertyPtr = reinterpret_cast(dataPtr); - *propertyPtr = std::move(*valuePtr); - return {}; - } - - return {}; + PropertyType *propertyPtr = static_cast(dataPtr); + *propertyPtr = impl(); } }; @@ -191,25 +181,12 @@ public: {} }; -namespace QtPrivate { - template - constexpr auto is_variant_v = false; - template - constexpr auto is_variant_v> = true; -} - namespace Qt { template auto makePropertyBinding(Functor &&f, const QPropertyBindingSourceLocation &location = QT_PROPERTY_DEFAULT_BINDING_LOCATION, std::enable_if_t> * = 0) { - if constexpr (QtPrivate::is_variant_v>) { - return QPropertyBinding>>(std::forward(f), location); - } else { - return QPropertyBinding>(std::forward(f), location); - } - // Work around bogus warning - Q_UNUSED(QtPrivate::is_variant_v); + return QPropertyBinding>(std::forward(f), location); } } diff --git a/src/corelib/kernel/qpropertybinding.cpp b/src/corelib/kernel/qpropertybinding.cpp index e05ef996b62..85dc59902a0 100644 --- a/src/corelib/kernel/qpropertybinding.cpp +++ b/src/corelib/kernel/qpropertybinding.cpp @@ -103,14 +103,12 @@ bool QPropertyBindingPrivate::evaluateIfDirtyAndReturnTrueIfValueChanged() BindingEvaluationState evaluationFrame(this); - QPropertyBindingError evalError; - QUntypedPropertyBinding::BindingEvaluationResult result; bool changed = false; if (metaType.id() == QMetaType::Bool) { auto propertyPtr = reinterpret_cast(propertyDataPtr); bool newValue = false; - evalError = evaluationFunction(metaType, &newValue); - if (evalError.type() == QPropertyBindingError::NoError) { + evaluationFunction(metaType, &newValue); + if (!error.hasError()) { bool updateAllowed = true; if (hasStaticObserver && staticGuardCallback) updateAllowed = staticGuardCallback(staticObserver, &newValue); @@ -121,8 +119,8 @@ bool QPropertyBindingPrivate::evaluateIfDirtyAndReturnTrueIfValueChanged() } } else { QVariant resultVariant(metaType.id(), nullptr); - evalError = evaluationFunction(metaType, resultVariant.data()); - if (evalError.type() == QPropertyBindingError::NoError) { + evaluationFunction(metaType, resultVariant.data()); + if (!error.hasError()) { bool updateAllowed = true; if (hasStaticObserver && staticGuardCallback) updateAllowed = staticGuardCallback(staticObserver, resultVariant.data()); @@ -134,9 +132,6 @@ bool QPropertyBindingPrivate::evaluateIfDirtyAndReturnTrueIfValueChanged() } } - if (evalError.type() != QPropertyBindingError::NoError) - error = evalError; - dirty = false; return changed; } diff --git a/src/corelib/kernel/qpropertybinding_p.h b/src/corelib/kernel/qpropertybinding_p.h index 6257e4506ea..7722a4b7c57 100644 --- a/src/corelib/kernel/qpropertybinding_p.h +++ b/src/corelib/kernel/qpropertybinding_p.h @@ -179,6 +179,11 @@ public: static QPropertyBindingPrivate *get(const QUntypedPropertyBinding &binding) { return binding.d.data(); } + + void setError(QPropertyBindingError &&e) + { error = std::move(e); } + + static QPropertyBindingPrivate *currentlyEvaluatingBinding(); }; QT_END_NAMESPACE diff --git a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp index 50a8d51a460..f14ba78b020 100644 --- a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp +++ b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp @@ -505,9 +505,10 @@ void tst_QProperty::bindingSourceLocation() void tst_QProperty::bindingError() { - QProperty prop = Qt::makePropertyBinding([]() -> std::variant { + QProperty prop = Qt::makePropertyBinding([]() -> int { QPropertyBindingError error(QPropertyBindingError::UnknownError, QLatin1String("my error")); - return error; + QPropertyBindingPrivate::currentlyEvaluatingBinding()->setError(std::move(error)); + return 0; }); QCOMPARE(prop.value(), 0); QCOMPARE(prop.binding().error().description(), QString("my error")); @@ -621,20 +622,18 @@ void tst_QProperty::genericPropertyBinding() { QUntypedPropertyBinding doubleBinding(QMetaType::fromType(), - [](const QMetaType &, void *) -> QUntypedPropertyBinding::BindingEvaluationResult { + [](const QMetaType &, void *) -> void { Q_ASSERT(false); - return QPropertyBindingError::NoError; }, QPropertyBindingSourceLocation()); QVERIFY(!property.setBinding(doubleBinding)); } QUntypedPropertyBinding intBinding(QMetaType::fromType(), - [](const QMetaType &metaType, void *dataPtr) -> QUntypedPropertyBinding::BindingEvaluationResult { + [](const QMetaType &metaType, void *dataPtr) -> void { Q_ASSERT(metaType.id() == qMetaTypeId()); int *intPtr = reinterpret_cast(dataPtr); *intPtr = 100; - return QPropertyBindingError::NoError; }, QPropertyBindingSourceLocation()); QVERIFY(property.setBinding(intBinding)); @@ -649,10 +648,9 @@ void tst_QProperty::genericPropertyBindingBool() QVERIFY(!property.value()); QUntypedPropertyBinding boolBinding(QMetaType::fromType(), - [](const QMetaType &, void *dataPtr) -> QUntypedPropertyBinding::BindingEvaluationResult { + [](const QMetaType &, void *dataPtr) -> void { auto boolPtr = reinterpret_cast(dataPtr); *boolPtr = true; - return {}; }, QPropertyBindingSourceLocation()); QVERIFY(property.setBinding(boolBinding));