QPromise/QFuture: fix value semantics

In random order:

* QFutureInterfaceBase has a d-pointer, and copy semantics. So,
"naturally" extend it to support move semantics as well. (These get
used by QPromise/QFuture, as they both hold a QFutureInterface*
as a data member). The only addition needed is a check for a null
d-pointer in the destructor.

Drive by, reorganize the code for the copies, and use copy-and-swap
instead of the hand-rolled solution. Also, add a free swap()
overload, and mark the existing one as candidate for inlining
in Qt 7 (doesn't need to be an exported function).

* QFutureInterface inherits QFutureInterfaceBase, again with value
semantics. To be honest, I'm not sure why QFutureInterfaceBase is
polymorphic -- could be a design mistake, as polymorphic classes
don't mix with value semantics. Anyways, reorganize the code for
copies, apply copy-and-swap, and add move semantics. This requires
adding a check into derefT().

* Finally, QPromise was already move-only, but had broken move
semantics: the move constructor was not noexcept (!) and it actually
allocated memory (!!!). Fix that one (can be defaulted now), and
streamline the move assignment via the proper macro.

Drive by, fix the signature of the constructor from QFutureInterface
(take const-ref, not plain ref -- it's eventually copied, so it
can keep the const), and add another internal constructor from
rvalue QFutureInterface that moves from it.

Change-Id: I9d61a9dd4d45f34942d8f34416baa118c0307390
Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>
Reviewed-by: Andrei Golubev <andrei.golubev@qt.io>
Reviewed-by: Sona Kurazyan <sona.kurazyan@qt.io>

src/corelib/thread/qfutureinterface.cpp

@@ -84,7 +84,7 @@ QFutureInterfaceBase::QFutureInterfaceBase(const QFutureInterfaceBase &other)
 
 QFutureInterfaceBase::~QFutureInterfaceBase()
 {
-    if (!d->refCount.deref())
+    if (d && !d->refCount.deref())
         delete d;
 }
 
@@ -394,6 +394,9 @@ bool QFutureInterfaceBase::queryState(State state) const
 
 int QFutureInterfaceBase::loadState() const
 {
+    // Used from ~QPromise, so this check is needed
+    if (!d)
+        return QFutureInterfaceBase::State::NoState;
     return d->state.loadRelaxed();
 }
 
@@ -568,13 +571,12 @@ const QtPrivate::ResultStoreBase &QFutureInterfaceBase::resultStoreBase() const
 
 QFutureInterfaceBase &QFutureInterfaceBase::operator=(const QFutureInterfaceBase &other)
 {
-    other.d->refCount.ref();
-    if (!d->refCount.deref())
-        delete d;
-    d = other.d;
+    QFutureInterfaceBase copy(other);
+    swap(copy);
     return *this;
 }
 
+// ### Qt 7: inline
 void QFutureInterfaceBase::swap(QFutureInterfaceBase &other) noexcept
 {
     qSwap(d, other.d);
@@ -587,7 +589,8 @@ bool QFutureInterfaceBase::refT() const
 
 bool QFutureInterfaceBase::derefT() const
 {
-    return d->refCount.derefT();
+    // Called from ~QFutureInterface
+    return !d || d->refCount.derefT();
 }
 
 void QFutureInterfaceBase::reset()

src/corelib/thread/qfutureinterface.h

@@ -95,6 +95,10 @@ public:
 
     QFutureInterfaceBase(State initialState = NoState);
     QFutureInterfaceBase(const QFutureInterfaceBase &other);
+    QFutureInterfaceBase(QFutureInterfaceBase &&other) noexcept
+        : d(std::exchange(other.d, nullptr)) {}
+    QFutureInterfaceBase &operator=(const QFutureInterfaceBase &other);
+    QT_MOVE_ASSIGNMENT_OPERATOR_IMPL_VIA_MOVE_AND_SWAP(QFutureInterfaceBase)
     virtual ~QFutureInterfaceBase();
 
     // reporting functions available to the engine author:
@@ -165,8 +169,8 @@ public:
 
     inline bool operator==(const QFutureInterfaceBase &other) const { return d == other.d; }
     inline bool operator!=(const QFutureInterfaceBase &other) const { return d != other.d; }
-    QFutureInterfaceBase &operator=(const QFutureInterfaceBase &other);
 
+    // ### Qt 7: inline
     void swap(QFutureInterfaceBase &other) noexcept;
 
 protected:
@@ -206,6 +210,11 @@ protected:
     bool isRunningOrPending() const;
 };
 
+inline void swap(QFutureInterfaceBase &lhs, QFutureInterfaceBase &rhs) noexcept
+{
+    lhs.swap(rhs);
+}
+
 template <typename T>
 class QFutureInterface : public QFutureInterfaceBase
 {
@@ -221,6 +230,16 @@ public:
         refT();
     }
     QFutureInterface(const QFutureInterfaceBase &dd) : QFutureInterfaceBase(dd) { refT(); }
+    QFutureInterface(QFutureInterfaceBase &&dd) : QFutureInterfaceBase(std::move(dd)) { refT(); }
+    QFutureInterface &operator=(const QFutureInterface &other)
+    {
+        QFutureInterface copy(other);
+        swap(copy);
+        return *this;
+    }
+    QFutureInterface(QFutureInterface &&other) = default;
+    QT_MOVE_ASSIGNMENT_OPERATOR_IMPL_VIA_MOVE_AND_SWAP(QFutureInterface)
+
     ~QFutureInterface()
     {
         if (!derefT())
@@ -230,15 +249,6 @@ public:
     static QFutureInterface canceledResult()
     { return QFutureInterface(State(Started | Finished | Canceled)); }
 
-    QFutureInterface &operator=(const QFutureInterface &other)
-    {
-        other.refT();
-        if (!derefT())
-            resultStoreBase().template clear<T>();
-        QFutureInterfaceBase::operator=(other);
-        return *this;
-    }
-
     inline QFuture<T> future(); // implemented in qfuture.h
 
     inline bool reportResult(const T *result, int index = -1);

src/corelib/thread/qpromise.h

@@ -58,17 +58,10 @@ class QPromise
 public:
     QPromise() = default;
     Q_DISABLE_COPY(QPromise)
-    QPromise(QPromise<T> &&other) : d(other.d)
-    {
-        other.d = QFutureInterface<T>();
-    }
-    QPromise(QFutureInterface<T> &other) : d(other) {}
-    QPromise& operator=(QPromise<T> &&other)
-    {
-        QPromise<T> tmp(std::move(other));
-        tmp.swap(*this);
-        return *this;
-    }
+    QPromise(QPromise<T> &&other) = default;
+    QPromise(const QFutureInterface<T> &other) : d(other) {}
+    QPromise(QFutureInterface<T> &&other) noexcept : d(std::move(other)) {}
+    QT_MOVE_ASSIGNMENT_OPERATOR_IMPL_VIA_MOVE_AND_SWAP(QPromise)
     ~QPromise()
     {
         const int state = d.loadState();

src/corelib/thread/qpromise.qdoc

@@ -78,7 +78,8 @@
     \sa operator=()
 */
 
-/*! \fn template <typename T> QPromise<T>::QPromise(QFutureInterface<T> &other)
+/*! \fn template <typename T> QPromise<T>::QPromise(const QFutureInterface<T> &other)
+    \fn template <typename T> QPromise<T>::QPromise(QFutureInterface<T> &&other)
 
     \internal
     Constructs a QPromise with a passed QFutureInterface \a other.