Skip to content

Commit

Permalink
Make QPropertyBindingPrivate accessible to QtQml
Browse files Browse the repository at this point in the history
QtQml needs the private just for one detail which nobody else should
need it for: Tracking additional dependencies and marking the binding as
dirty. Exporting the private requires hiding some variables and
providing accessors, to compile with MSVC - including the removal of
QVarLengthArray usage. Upside: The binding structure shrinks by 8 bytes
and the encapsulation makes it a little easier to change things without
breaking declarative, ... in the unlikely event ;-)

Also remove setDirty() from the public API as it's not needed by QtQml
and using it is dangerous, because it means that there's a risk of
somebody keeping a reference (count) to the untyped binding from within
the binding closure, which introduces a memory leak.

Change-Id: I43bd56f4bdf218efb54fa23e2d627ad3acfafeb5
Reviewed-by: Fabian Kosmale <[email protected]>
  • Loading branch information
tronical committed Mar 27, 2020
1 parent 96de3e2 commit f3ce9e9
Show file tree
Hide file tree
Showing 7 changed files with 64 additions and 43 deletions.
24 changes: 9 additions & 15 deletions src/corelib/kernel/qproperty.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ QPropertyBase::QPropertyBase(QPropertyBase &&other, void *propertyDataPtr)
QPropertyBasePointer d{this};
d.setFirstObserver(nullptr);
if (auto binding = d.bindingPtr())
binding->propertyDataPtr = propertyDataPtr;
binding->setProperty(propertyDataPtr);
}

void QPropertyBase::moveAssign(QPropertyBase &&other, void *propertyDataPtr)
Expand All @@ -73,7 +73,7 @@ void QPropertyBase::moveAssign(QPropertyBase &&other, void *propertyDataPtr)
std::swap(d_ptr, other.d_ptr);

if (auto binding = d.bindingPtr())
binding->propertyDataPtr = propertyDataPtr;
binding->setProperty(propertyDataPtr);

d.setFirstObserver(observer.ptr);

Expand Down Expand Up @@ -111,10 +111,10 @@ QUntypedPropertyBinding QPropertyBase::setBinding(const QUntypedPropertyBinding
newBinding.data()->ref.ref();
d_ptr = (d_ptr & FlagMask) | reinterpret_cast<quintptr>(newBinding.data());
d_ptr |= BindingBit;
newBinding->dirty = true;
newBinding->propertyDataPtr = propertyDataPtr;
newBinding->setDirty(true);
newBinding->setProperty(propertyDataPtr);
if (observer)
observer.prependToBinding(newBinding.data());
newBinding->prependObserver(observer);
} else {
d_ptr &= ~BindingBit;
}
Expand Down Expand Up @@ -175,11 +175,10 @@ static thread_local BindingEvaluationState *currentBindingEvaluationState = null

BindingEvaluationState::BindingEvaluationState(QPropertyBindingPrivate *binding)
: binding(binding)
, dependencyObservers(&binding->dependencyObservers)
{
previousState = currentBindingEvaluationState;
currentBindingEvaluationState = this;
dependencyObservers->clear();
binding->clearDependencyObservers();
}

BindingEvaluationState::~BindingEvaluationState()
Expand Down Expand Up @@ -222,8 +221,7 @@ void QPropertyBase::registerWithCurrentlyEvaluatingBinding() const

QPropertyBasePointer d{this};

currentState->dependencyObservers->append(QPropertyObserver());
QPropertyObserverPointer dependencyObserver{&(*currentState->dependencyObservers)[currentState->dependencyObservers->size() - 1]};
QPropertyObserverPointer dependencyObserver = currentState->binding->allocateDependencyObserver();
dependencyObserver.setBindingToMarkDirty(currentState->binding);
dependencyObserver.observeProperty(d);
}
Expand Down Expand Up @@ -263,6 +261,8 @@ QPropertyObserver::~QPropertyObserver()
d.unlink();
}

QPropertyObserver::QPropertyObserver() = default;

QPropertyObserver::QPropertyObserver(QPropertyObserver &&other)
{
std::swap(bindingToMarkDirty, other.bindingToMarkDirty);
Expand Down Expand Up @@ -351,12 +351,6 @@ void QPropertyObserverPointer::observeProperty(QPropertyBasePointer property)
property.addObserver(ptr);
}

void QPropertyObserverPointer::prependToBinding(QPropertyBindingPrivate *binding)
{
ptr->prev = const_cast<QPropertyObserver **>(&binding->firstObserver.ptr);
binding->firstObserver = *this;
}

QPropertyBindingError::QPropertyBindingError(Type type)
{
if (type != NoError) {
Expand Down
14 changes: 7 additions & 7 deletions src/corelib/kernel/qproperty.h
Original file line number Diff line number Diff line change
Expand Up @@ -137,12 +137,10 @@ class Q_CORE_EXPORT QUntypedPropertyBinding

QMetaType valueMetaType() const;

void setDirty(bool dirty = true);

private:
explicit QUntypedPropertyBinding(const QPropertyBindingPrivatePtr &priv);
private:
friend class QtPrivate::QPropertyBase;
friend struct QPropertyBindingPrivate;
friend class QPropertyBindingPrivate;
template <typename> friend class QPropertyBinding;
QPropertyBindingPrivatePtr d;
};
Expand Down Expand Up @@ -357,7 +355,7 @@ class QProperty

friend struct QPropertyBasePointer;
friend class QPropertyBinding<T>;
friend struct QPropertyObserver;
friend class QPropertyObserver;
// Mutable because querying for the value may require evalating the binding expression, calling
// non-const functions on QPropertyBase.
mutable QtPrivate::QPropertyValueStorage<T> d;
Expand Down Expand Up @@ -385,16 +383,17 @@ namespace Qt {
struct QPropertyObserverPrivate;
struct QPropertyObserverPointer;

struct Q_CORE_EXPORT QPropertyObserver
class Q_CORE_EXPORT QPropertyObserver
{
public:
// Internal
enum ObserverTag {
ObserverNotifiesBinding = 0x0,
ObserverNotifiesChangeHandler = 0x1,
};
Q_DECLARE_FLAGS(ObserverTags, ObserverTag)

QPropertyObserver() = default;
QPropertyObserver();
QPropertyObserver(QPropertyObserver &&other);
QPropertyObserver &operator=(QPropertyObserver &&other);
~QPropertyObserver();
Expand Down Expand Up @@ -424,6 +423,7 @@ struct Q_CORE_EXPORT QPropertyObserver

friend struct QPropertyObserverPointer;
friend struct QPropertyBasePointer;
friend class QPropertyBindingPrivate;
};

Q_DECLARE_OPERATORS_FOR_FLAGS(QPropertyObserver::ObserverTags)
Expand Down
2 changes: 0 additions & 2 deletions src/corelib/kernel/qproperty_p.h
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ struct QPropertyObserverPointer

void notify(QPropertyBindingPrivate *triggeringBinding);
void observeProperty(QPropertyBasePointer property);
void prependToBinding(QPropertyBindingPrivate *binding);

explicit operator bool() const { return ptr != nullptr; }

Expand All @@ -110,7 +109,6 @@ struct BindingEvaluationState
BindingEvaluationState(QPropertyBindingPrivate *binding);
~BindingEvaluationState();
QPropertyBindingPrivate *binding;
QVarLengthArray<QPropertyObserver, 4> *dependencyObservers = nullptr;
BindingEvaluationState *previousState = nullptr;
};

Expand Down
11 changes: 2 additions & 9 deletions src/corelib/kernel/qpropertybinding.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -152,21 +152,14 @@ QPropertyBindingError QUntypedPropertyBinding::error() const
{
if (!d)
return QPropertyBindingError();
return d->error;
return d->bindingError();
}

QMetaType QUntypedPropertyBinding::valueMetaType() const
{
if (!d)
return QMetaType();
return d->metaType;
}

void QUntypedPropertyBinding::setDirty(bool dirty)
{
if (!d)
return;
d->dirty = dirty;
return d->valueMetaType();
}

QT_END_NAMESPACE
46 changes: 42 additions & 4 deletions src/corelib/kernel/qpropertybinding_p.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,21 +53,24 @@

#include <QtCore/qglobal.h>
#include <QtCore/qshareddata.h>
#include <QtCore/qvarlengtharray.h>
#include <memory>
#include <QtCore/qscopedpointer.h>
#include <vector>
#include <functional>

#include "qproperty_p.h"

QT_BEGIN_NAMESPACE

struct QPropertyBindingPrivate : public QSharedData
class Q_CORE_EXPORT QPropertyBindingPrivate : public QSharedData
{
private:
friend struct QPropertyBasePointer;

QUntypedPropertyBinding::BindingEvaluationFunction evaluationFunction;

QPropertyObserverPointer firstObserver;
QVarLengthArray<QPropertyObserver, 4> dependencyObservers;
std::array<QPropertyObserver, 4> inlineDependencyObservers;
QScopedPointer<std::vector<QPropertyObserver>> heapObservers;

void *propertyDataPtr = nullptr;

Expand All @@ -79,6 +82,10 @@ struct QPropertyBindingPrivate : public QSharedData
bool dirty = false;
bool updating = false;

public:
// public because the auto-tests access it, too.
size_t dependencyObserverCount = 0;

QPropertyBindingPrivate(const QMetaType &metaType, QUntypedPropertyBinding::BindingEvaluationFunction evaluationFunction,
const QPropertyBindingSourceLocation &location)
: evaluationFunction(std::move(evaluationFunction))
Expand All @@ -87,6 +94,37 @@ struct QPropertyBindingPrivate : public QSharedData
{}
virtual ~QPropertyBindingPrivate();

void setDirty(bool d) { dirty = d; }
void setProperty(void *propertyPtr) { propertyDataPtr = propertyPtr; }
void prependObserver(QPropertyObserverPointer observer) {
observer.ptr->prev = const_cast<QPropertyObserver **>(&firstObserver.ptr);
firstObserver = observer;
}

void clearDependencyObservers() {
for (size_t i = 0; i < inlineDependencyObservers.size(); ++i) {
QPropertyObserver empty;
qSwap(inlineDependencyObservers[i], empty);
}
if (heapObservers)
heapObservers->clear();
dependencyObserverCount = 0;
}
QPropertyObserverPointer allocateDependencyObserver() {
if (dependencyObserverCount < inlineDependencyObservers.size()) {
++dependencyObserverCount;
return {&inlineDependencyObservers[dependencyObserverCount - 1]};
}
++dependencyObserverCount;
if (!heapObservers)
heapObservers.reset(new std::vector<QPropertyObserver>());
return {&heapObservers->emplace_back()};
}

QPropertyBindingSourceLocation sourceLocation() const { return location; }
QPropertyBindingError bindingError() const { return error; }
QMetaType valueMetaType() const { return metaType; }

void unlinkAndDeref();

void markDirtyAndNotifyObservers();
Expand Down
2 changes: 1 addition & 1 deletion src/corelib/kernel/qpropertyprivate.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
QT_BEGIN_NAMESPACE

class QUntypedPropertyBinding;
struct QPropertyBindingPrivate;
class QPropertyBindingPrivate;
using QPropertyBindingPrivatePtr = QExplicitlySharedDataPointer<QPropertyBindingPrivate>;
struct QPropertyBasePointer;

Expand Down
8 changes: 3 additions & 5 deletions tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -220,13 +220,11 @@ void tst_QProperty::avoidDependencyAllocationAfterFirstEval()
QCOMPARE(propWithBinding.value(), int(11));

QVERIFY(QPropertyBasePointer::get(propWithBinding).bindingPtr());
QCOMPARE(QPropertyBasePointer::get(propWithBinding).bindingPtr()->dependencyObservers.size(), 2);
QVERIFY(QPropertyBasePointer::get(propWithBinding).bindingPtr()->dependencyObservers.capacity() >= 2);
QCOMPARE(QPropertyBasePointer::get(propWithBinding).bindingPtr()->dependencyObserverCount, 2);

firstDependency = 100;
QCOMPARE(propWithBinding.value(), int(110));
QCOMPARE(QPropertyBasePointer::get(propWithBinding).bindingPtr()->dependencyObservers.size(), 2);
QVERIFY(QPropertyBasePointer::get(propWithBinding).bindingPtr()->dependencyObservers.capacity() >= 2);
QCOMPARE(QPropertyBasePointer::get(propWithBinding).bindingPtr()->dependencyObserverCount, 2);
}

void tst_QProperty::propertyArrays()
Expand Down Expand Up @@ -493,7 +491,7 @@ void tst_QProperty::bindingSourceLocation()
#if defined(QT_PROPERTY_COLLECT_BINDING_LOCATION)
auto bindingLine = std::experimental::source_location::current().line() + 1;
auto binding = Qt::makePropertyBinding([]() { return 42; });
QCOMPARE(QPropertyBindingPrivate::get(binding)->location.line, bindingLine);
QCOMPARE(QPropertyBindingPrivate::get(binding)->sourceLocation().line, bindingLine);
#else
QSKIP("Skipping this in the light of missing binding source location support");
#endif
Expand Down

0 comments on commit f3ce9e9

Please sign in to comment.