Use QTaggedPointer in QPropertyObserver

This replaces the private tagged pointer and the use of enums for the
tag makes the observer handling code more readable. The
pointer-to-tagged-pointer class remains in qpropertyprivate.h due to its
exoticness.

Change-Id: Icc88799136c6839426d994b42368526463265e66
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

src/corelib/kernel/qproperty.cpp

@@ -51,7 +51,7 @@ QPropertyBase::QPropertyBase(QPropertyBase &&other, void *propertyDataPtr)
 {
     std::swap(d_ptr, other.d_ptr);
     QPropertyBasePointer d{this};
-    d.firstObserverPtr().set(nullptr);
+    d.setFirstObserver(nullptr);
     if (auto binding = d.bindingPtr())
         binding->propertyDataPtr = propertyDataPtr;
 }
@@ -63,7 +63,7 @@ void QPropertyBase::moveAssign(QPropertyBase &&other, void *propertyDataPtr)
 
     QPropertyBasePointer d{this};
     auto observer = d.firstObserver();
-    d.firstObserverPtr().set(nullptr);
+    d.setFirstObserver(nullptr);
 
     if (auto binding = d.bindingPtr()) {
         binding->unlinkAndDeref();
@@ -75,7 +75,7 @@ void QPropertyBase::moveAssign(QPropertyBase &&other, void *propertyDataPtr)
     if (auto binding = d.bindingPtr())
         binding->propertyDataPtr = propertyDataPtr;
 
-    d.firstObserverPtr().set(const_cast<QPropertyObserver*>(observer.ptr));
+    d.setFirstObserver(observer.ptr);
 
     // The caller will have to notify observers.
 }
@@ -137,11 +137,31 @@ QPropertyBindingPrivate *QPropertyBasePointer::bindingPtr() const
     return nullptr;
 }
 
-QtPrivate::QPropertyTagPreservingPointerToPointer<QPropertyObserver> QPropertyBasePointer::firstObserverPtr() const
+void QPropertyBasePointer::addObserver(QPropertyObserver *observer)
 {
-    if (auto *binding = bindingPtr())
-        return const_cast<QPropertyObserver**>(&binding->firstObserver.ptr);
-    return &ptr->d_ptr;
+    if (auto *binding = bindingPtr()) {
+        observer->prev = &binding->firstObserver.ptr;
+        observer->next = binding->firstObserver.ptr;
+        if (observer->next)
+            observer->next->prev = &observer->next;
+        binding->firstObserver.ptr = observer;
+    } else {
+        auto firstObserver = reinterpret_cast<QPropertyObserver*>(ptr->d_ptr & ~QPropertyBase::FlagMask);
+        observer->prev = reinterpret_cast<QPropertyObserver**>(&ptr->d_ptr);
+        observer->next = firstObserver;
+        if (observer->next)
+            observer->next->prev = &observer->next;
+    }
+    setFirstObserver(observer);
+}
+
+void QPropertyBasePointer::setFirstObserver(QPropertyObserver *observer)
+{
+    if (auto *binding = bindingPtr()) {
+        binding->firstObserver.ptr = observer;
+        return;
+    }
+    ptr->d_ptr = reinterpret_cast<quintptr>(observer) | (ptr->d_ptr & QPropertyBase::FlagMask);
 }
 
 QPropertyObserverPointer QPropertyBasePointer::firstObserver() const
@@ -249,9 +269,9 @@ QPropertyObserver::QPropertyObserver(QPropertyObserver &&other)
     std::swap(next, other.next);
     std::swap(prev, other.prev);
     if (next)
-        next->prev = reinterpret_cast<quintptr*>(&next);
+        next->prev = &next;
     if (prev)
-        prev.set(this);
+        prev.setPointer(this);
 }
 
 QPropertyObserver &QPropertyObserver::operator=(QPropertyObserver &&other)
@@ -267,9 +287,9 @@ QPropertyObserver &QPropertyObserver::operator=(QPropertyObserver &&other)
     std::swap(next, other.next);
     std::swap(prev, other.prev);
     if (next)
-        next->prev = reinterpret_cast<quintptr*>(&next);
+        next->prev = &next;
     if (prev)
-        prev.set(this);
+        prev.setPointer(this);
 
     return *this;
 }
@@ -279,7 +299,7 @@ void QPropertyObserverPointer::unlink()
     if (ptr->next)
         ptr->next->prev = ptr->prev;
     if (ptr->prev)
-        ptr->prev.set(ptr->next.data());
+        ptr->prev.setPointer(ptr->next.pointer());
     ptr->next = nullptr;
     ptr->prev.clear();
 }
@@ -287,13 +307,13 @@ void QPropertyObserverPointer::unlink()
 void QPropertyObserverPointer::setChangeHandler(void (*changeHandler)(QPropertyObserver *))
 {
     ptr->changeHandler = changeHandler;
-    ptr->next.setFlag(true);
+    ptr->next.setTag(QPropertyObserver::ObserverNotifiesChangeHandler);
 }
 
 void QPropertyObserverPointer::setBindingToMarkDirty(QPropertyBindingPrivate *binding)
 {
     ptr->bindingToMarkDirty = binding;
-    ptr->next.setFlag(false);
+    ptr->next.setTag(QPropertyObserver::ObserverNotifiesBinding);
 }
 
 void QPropertyObserverPointer::notify(QPropertyBindingPrivate *triggeringBinding)
@@ -303,8 +323,8 @@ void QPropertyObserverPointer::notify(QPropertyBindingPrivate *triggeringBinding
 
     auto observer = const_cast<QPropertyObserver*>(ptr);
     while (observer) {
-        auto * const next = observer->next.data();
-        if (observer->next.flag()) {
+        auto * const next = observer->next.pointer();
+        if (observer->next.tag() == QPropertyObserver::ObserverNotifiesChangeHandler) {
             if (!knownIfPropertyChanged && triggeringBinding) {
                 knownIfPropertyChanged = true;
 
@@ -328,12 +348,7 @@ void QPropertyObserverPointer::notify(QPropertyBindingPrivate *triggeringBinding
 void QPropertyObserverPointer::observeProperty(QPropertyBasePointer property)
 {
     unlink();
-    auto firstObserverPtr = property.firstObserverPtr();
-    ptr->prev = firstObserverPtr;
-    ptr->next = firstObserverPtr.get();
-    if (ptr->next)
-        ptr->next->prev = &ptr->next;
-    firstObserverPtr.set(ptr);
+    property.addObserver(ptr);
 }
 
 void QPropertyObserverPointer::prependToBinding(QPropertyBindingPrivate *binding)

src/corelib/kernel/qproperty.h

@@ -387,6 +387,13 @@ struct QPropertyObserverPointer;
 
 struct Q_CORE_EXPORT QPropertyObserver
 {
+    // Internal
+    enum ObserverTag {
+        ObserverNotifiesBinding = 0x0,
+        ObserverNotifiesChangeHandler = 0x1,
+    };
+    Q_DECLARE_FLAGS(ObserverTags, ObserverTag)
+
     QPropertyObserver() = default;
     QPropertyObserver(QPropertyObserver &&other);
     QPropertyObserver &operator=(QPropertyObserver &&other);
@@ -402,10 +409,10 @@ protected:
 private:
     void setSource(QtPrivate::QPropertyBase &property);
 
-    QtPrivate::QTaggedPointer<QPropertyObserver> next;
+    QTaggedPointer<QPropertyObserver, ObserverTags> next;
     // prev is a pointer to the "next" element within the previous node, or to the "firstObserverPtr" if it is the
     // first node.
-    QtPrivate::QPropertyTagPreservingPointerToPointer<QPropertyObserver> prev;
+    QtPrivate::QTagPreservingPointerToPointer<QPropertyObserver, ObserverTags> prev;
 
     union {
         QPropertyBindingPrivate *bindingToMarkDirty = nullptr;
@@ -416,8 +423,11 @@ private:
     QPropertyObserver &operator=(const QPropertyObserver &) = delete;
 
     friend struct QPropertyObserverPointer;
+    friend struct QPropertyBasePointer;
 };
 
+Q_DECLARE_OPERATORS_FOR_FLAGS(QPropertyObserver::ObserverTags)
+
 template <typename Functor>
 class QPropertyChangeHandler : public QPropertyObserver
 {

src/corelib/kernel/qproperty_p.h

@@ -65,7 +65,8 @@ struct Q_AUTOTEST_EXPORT QPropertyBasePointer
 
     QPropertyBindingPrivate *bindingPtr() const;
 
-    QtPrivate::QPropertyTagPreservingPointerToPointer<QPropertyObserver> firstObserverPtr() const;
+    void addObserver(QPropertyObserver *observer);
+    void setFirstObserver(QPropertyObserver *observer);
     QPropertyObserverPointer firstObserver() const;
 
     int observerCount() const;
@@ -93,7 +94,7 @@ struct QPropertyObserverPointer
 
     explicit operator bool() const { return ptr != nullptr; }
 
-    QPropertyObserverPointer nextObserver() const { return {ptr->next.data()}; }
+    QPropertyObserverPointer nextObserver() const { return {ptr->next.pointer()}; }
 };
 
 class QPropertyBindingErrorPrivate : public QSharedData

src/corelib/kernel/qpropertyprivate.h

@@ -53,6 +53,7 @@
 
 #include <QtCore/qglobal.h>
 #include <QtCore/QExplicitlySharedDataPointer>
+#include <QtCore/qtaggedpointer.h>
 
 QT_BEGIN_NAMESPACE
 
@@ -161,85 +162,50 @@ struct QPropertyValueStorage<bool>
     }
 };
 
-template <typename T> class QTaggedPointer;
-
-template <typename T, quintptr Mask = 0x3>
-class QPropertyTagPreservingPointerToPointer
+template <typename T, typename Tag>
+class QTagPreservingPointerToPointer
 {
-    quintptr *data = nullptr;
-
 public:
-    QPropertyTagPreservingPointerToPointer() = default;
-    QPropertyTagPreservingPointerToPointer(T **ptr)
-        : data(reinterpret_cast<quintptr*>(ptr))
-    {}
-    QPropertyTagPreservingPointerToPointer(quintptr *ptr)
-        : data(ptr)
+    QTagPreservingPointerToPointer() = default;
+
+    QTagPreservingPointerToPointer(T **ptr)
+        : d(reinterpret_cast<quintptr*>(ptr))
     {}
 
-    QPropertyTagPreservingPointerToPointer<T> &operator=(T **ptr)
+    QTagPreservingPointerToPointer<T, Tag> &operator=(T **ptr)
     {
-        data = reinterpret_cast<quintptr*>(ptr);
+        d = reinterpret_cast<quintptr*>(ptr);
         return *this;
     }
 
-    QPropertyTagPreservingPointerToPointer<T> &operator=(QTaggedPointer<T> *ptr)
+    QTagPreservingPointerToPointer<T, Tag> &operator=(QTaggedPointer<T, Tag> *ptr)
     {
-        data = reinterpret_cast<quintptr*>(ptr);
+        d = reinterpret_cast<quintptr*>(ptr);
         return *this;
     }
 
     void clear()
     {
-        data = nullptr;
+        d = nullptr;
     }
 
-    void set(T *ptr)
+    void setPointer(T *ptr)
     {
-        *data = (*data & Mask) | (reinterpret_cast<quintptr>(ptr) & ~Mask);
+        *d = (reinterpret_cast<quintptr>(ptr) & QTaggedPointer<T, Tag>::pointerMask()) | (*d & QTaggedPointer<T, Tag>::tagMask());
     }
 
     T *get() const
     {
-        return reinterpret_cast<T*>(*data & ~Mask);
+        return reinterpret_cast<T*>(*d & QTaggedPointer<T, Tag>::pointerMask());
     }
 
-    explicit operator bool() const { return data != nullptr; }
-};
-
-template <typename T>
-class QTaggedPointer
-{
-public:
-    QTaggedPointer() = default;
-    QTaggedPointer(T *ptr) : tagAndPointer(reinterpret_cast<quintptr>(ptr)) {}
-
-    void setFlag(bool b)
+    explicit operator bool() const
     {
-        if (b)
-            tagAndPointer |= Flag1Mask;
-        else
-            tagAndPointer &= ~Flag1Mask;
+        return d != nullptr;
     }
 
-    bool flag() const { return tagAndPointer & Flag1Mask; }
-
-    QTaggedPointer &operator=(T *ptr)
-    {
-        quintptr tag = tagAndPointer & TagMask;
-        tagAndPointer = reinterpret_cast<quintptr>(ptr) | tag;
-        return *this;
-    }
-
-    T *data() const { return reinterpret_cast<T*>(tagAndPointer & ~TagMask); }
-
-    explicit operator bool() const { return tagAndPointer & ~TagMask; }
-    T *operator->() const { return data(); }
-
 private:
-    quintptr tagAndPointer = 0;
-    static const quintptr Flag1Mask = 0x1;
-    static const quintptr TagMask = 0x3;
+    quintptr *d = nullptr;
 };
 
 } // namespace QtPrivate

src/corelib/tools/qtaggedpointer.h

@@ -73,13 +73,13 @@ namespace QtPrivate {
 template <typename T, typename Tag = typename QtPrivate::TagInfo<T>::TagType>
 class QTaggedPointer
 {
-    static constexpr quintptr tagMask() { return QtPrivate::TagInfo<T>::alignment - 1; }
-    static constexpr quintptr pointerMask() { return ~tagMask(); }
-
 public:
     using Type = T;
     using TagType = Tag;
 
+    static constexpr quintptr tagMask() { return QtPrivate::TagInfo<T>::alignment - 1; }
+    static constexpr quintptr pointerMask() { return ~tagMask(); }
+
     explicit QTaggedPointer(Type *pointer = nullptr, TagType tag = TagType()) noexcept
         : d(quintptr(pointer))
     {