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 <ulf.hermann@qt.io>

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};
@@ -694,12 +667,6 @@ QString QPropertyBindingError::description() const
 */
 
 /*!
-  \fn template <typename T> QProperty<T> &QProperty<T>::operator=(QProperty &&other)
-
-  Move-assigns \a other to this QProperty instance.
-*/
-
-/*!
   \fn template <typename T> QProperty<T>::QProperty(const QPropertyBinding<T> &binding)
 
   Constructs a property that is tied to the provided \a binding expression. The
@@ -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<T>(initialValue) {}
     explicit QProperty(rvalue_ref initialValue) : QPropertyData<T>(std::move(initialValue)) {}
-    QProperty(QProperty &&other) : QPropertyData<T>(std::move(other.val)), d(std::move(other.d), this) { notify(); }
     explicit QProperty(const QPropertyBinding<T> &binding)
         : QProperty()
     { setBinding(binding); }
@@ -329,13 +328,6 @@ public:
     template <typename Functor>
     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<QPropertyObserver**>(&(ptr->d_ptr));
         ptr->d_ptr = (reinterpret_cast<quintptr>(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<quintptr>(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<QPropertyObserver**>(&(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<QProperty<int>> properties;
-
-    int expectedSum = 0;
-    for (int i = 0; i < 10; ++i) {
-        properties.emplace_back(i);
-        expectedSum += i;
-    }
-
-    QProperty<int> 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<bool> first(true);
@@ -311,74 +286,6 @@ void tst_QProperty::replaceBinding()
     QCOMPARE(second.value(), 100);
 }
 
-void tst_QProperty::swap()
-{
-    QProperty<int> firstDependency(1);
-    QProperty<int> secondDependency(2);
-
-    QProperty<int> first(Qt::makePropertyBinding(firstDependency));
-    QProperty<int> 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<int> first(1);
-    QProperty<int> second(2);
-
-    QProperty<int> propertyInTheMiddle(Qt::makePropertyBinding(first));
-
-    QProperty<int> finalProp1(Qt::makePropertyBinding(propertyInTheMiddle));
-    QProperty<int> finalProp2(Qt::makePropertyBinding(propertyInTheMiddle));
-
-    QCOMPARE(finalProp1.value(), 1);
-    QCOMPARE(finalProp2.value(), 1);
-
-    QCOMPARE(QPropertyBindingDataPointer::get(propertyInTheMiddle).observerCount(), 2);
-
-    QProperty<int> other(Qt::makePropertyBinding(second));
-    QCOMPARE(other.value(), 2);
-
-    QProperty<int> 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<int> first(1);
-
-    QProperty<int> intermediate(Qt::makePropertyBinding(first));
-    QCOMPARE(intermediate.value(), 1);
-    QCOMPARE(QPropertyBindingDataPointer::get(first).observerCount(), 1);
-
-    QProperty<int> targetProp(std::move(first));
-
-    QCOMPARE(QPropertyBindingDataPointer::get(targetProp).observerCount(), 0);
-}
-
 void tst_QProperty::changeHandler()
 {
     QProperty<int> 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()