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>

4 files changed, 35 insertions, 28 deletions

diff --git a/src/corelib/thread/qfutureinterface.cpp b/src/corelib/thread/qfutureinterface.cpp
index 76af95e3a3..22c8a48411 100644
--- a/src/corelib/thread/qfutureinterface.cpp
+++ b/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()
diff --git a/src/corelib/thread/qfutureinterface.h b/src/corelib/thread/qfutureinterface.h
index 62875c5ef9..4da5b36eae 100644
--- a/src/corelib/thread/qfutureinterface.h
+++ b/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);
diff --git a/src/corelib/thread/qpromise.h b/src/corelib/thread/qpromise.h
index e453f29b6a..24b0cd1e80 100644
--- a/src/corelib/thread/qpromise.h
+++ b/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();
diff --git a/src/corelib/thread/qpromise.qdoc b/src/corelib/thread/qpromise.qdoc
index 78182323be..f051faf9f9 100644
--- a/src/corelib/thread/qpromise.qdoc
+++ b/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.