QVariant::fromValue: Add rvalue optimization

When passing an rvalue-reference to QVariant, there is no reason to make
a copy if the type is moveable. Moreover, we know that the pointer which
we construct from the object passed to fromValue non-null. We make use
of both facts by parametrizing custom_construct on
non-nullness and availability of a move-ctor, and then dispatching to
the suitable template.
We need to keep the const T& overload, as otherwise code which
explicitly specializes fromValue and passes a const lvalue to it would
stop to compile.

[ChangeLog][QtCore][QVariant] Added fromValue() overload taking rvalues.

Change-Id: I44fb757d516ef364fe7967bc103b3f98278b4919
Reviewed-by: Marc Mutz <marc.mutz@qt.io>
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

src/corelib/kernel/qmetatype_p.h

@@ -178,6 +178,15 @@ inline void copyConstruct(const QtPrivate::QMetaTypeInterface *iface, void *wher
         memcpy(where, copy, iface->size);
 }
 
+inline void moveConstruct(const QtPrivate::QMetaTypeInterface *iface, void *where, void *copy)
+{
+    Q_ASSERT(isMoveConstructible(iface));
+    if (iface->moveCtr)
+        iface->moveCtr(iface, where, copy);
+    else
+        memcpy(where, copy, iface->size);
+}
+
 inline void construct(const QtPrivate::QMetaTypeInterface *iface, void *where, const void *copy)
 {
     if (copy)

src/corelib/kernel/qvariant.cpp

@@ -47,6 +47,8 @@
 #include "qline.h"
 #endif
 
+#include <memory>
+
 #include <cmath>
 #include <float.h>
 #include <cstring>
@@ -231,9 +233,38 @@ static bool isValidMetaTypeForVariant(const QtPrivate::QMetaTypeInterface *iface
     return true;
 }
 
+enum CustomConstructMoveOptions {
+    UseCopy,  // custom construct uses the copy ctor unconditionally
+    // future option: TryMove: uses move ctor if available, else copy ctor
+    ForceMove, // custom construct use the move ctor (which must exist)
+};
+
+enum CustomConstructNullabilityOption {
+    MaybeNull, // copy might be null, might be non-null
+    NonNull, // copy is guarantueed to be non-null
+    // future option: AlwaysNull?
+};
+
+
+template <typename F> static QVariant::PrivateShared *
+customConstructShared(size_t size, size_t align, F &&construct)
+{
+    struct Deleter {
+        void operator()(QVariant::PrivateShared *p) const
+        { QVariant::PrivateShared::free(p); }
+    };
+
+    // this is exception-safe
+    std::unique_ptr<QVariant::PrivateShared, Deleter> ptr;
+    ptr.reset(QVariant::PrivateShared::create(size, align));
+    construct(ptr->data());
+    return ptr.release();
+}
+
 // the type of d has already been set, but other field are not set
+template <CustomConstructMoveOptions moveOption = UseCopy, CustomConstructNullabilityOption nullability = MaybeNull>
 static void customConstruct(const QtPrivate::QMetaTypeInterface *iface, QVariant::Private *d,
-                            const void *copy)
+                            std::conditional_t<moveOption == ForceMove, void *, const void *> copy)
 {
     using namespace QtMetaTypePrivate;
     Q_ASSERT(iface);
@@ -242,6 +273,10 @@ static void customConstruct(const QtPrivate::QMetaTypeInterface *iface, QVariant
     Q_ASSERT(isCopyConstructible(iface));
     Q_ASSERT(isDestructible(iface));
     Q_ASSERT(copy || isDefaultConstructible(iface));
+    if constexpr (moveOption == ForceMove)
+        Q_ASSERT(isMoveConstructible(iface));
+    if constexpr (nullability == NonNull)
+        Q_ASSUME(copy != nullptr);
 
     // need to check for nullptr_t here, as this can get called by fromValue(nullptr). fromValue() uses
     // std::addressof(value) which in this case returns the address of the nullptr object.
@@ -251,11 +286,17 @@ static void customConstruct(const QtPrivate::QMetaTypeInterface *iface, QVariant
     if (QVariant::Private::canUseInternalSpace(iface)) {
         d->is_shared = false;
         if (!copy && !iface->defaultCtr)
-            return;     // default constructor and it's OK to build in 0-filled storage, which we've already done
-        construct(iface, d->data.data, copy);
+            return;     // trivial default constructor and it's OK to build in 0-filled storage, which we've already done
+        if constexpr (moveOption == ForceMove && nullability == NonNull)
+            moveConstruct(iface, d->data.data, copy);
+        else
+            construct(iface, d->data.data, copy);
     } else {
         d->data.shared = customConstructShared(iface->size, iface->alignment, [=](void *where) {
-            construct(iface, where, copy);
+            if constexpr (moveOption == ForceMove && nullability == NonNull)
+                moveConstruct(iface, where, copy);
+            else
+                construct(iface, where, copy);
         });
         d->is_shared = true;
     }
@@ -1064,7 +1105,8 @@ void QVariant::detach()
 
     Q_ASSERT(isValidMetaTypeForVariant(d.typeInterface(), constData()));
     Private dd(d.typeInterface());
-    customConstruct(d.typeInterface(), &dd, constData());
+    // null variant is never shared; anything else is NonNull
+    customConstruct<UseCopy, NonNull>(d.typeInterface(), &dd, constData());
     if (!d.data.shared->ref.deref())
         customClear(&d);
     d.data.shared = dd.data.shared;
@@ -2524,6 +2566,22 @@ QDebug QVariant::qdebugHelper(QDebug dbg) const
     return dbg;
 }
 
+QVariant QVariant::moveConstruct(QMetaType type, void *data)
+{
+    QVariant var;
+    var.d = QVariant::Private(type.d_ptr);
+    customConstruct<ForceMove, NonNull>(type.d_ptr, &var.d, data);
+    return var;
+}
+
+QVariant QVariant::copyConstruct(QMetaType type, const void *data)
+{
+    QVariant var;
+    var.d = QVariant::Private(type.d_ptr);
+    customConstruct<UseCopy, NonNull>(type.d_ptr, &var.d, data);
+    return var;
+}
+
 #if QT_DEPRECATED_SINCE(6, 0)
 QT_WARNING_PUSH
 QT_WARNING_DISABLE_DEPRECATED
@@ -2644,6 +2702,12 @@ QT_WARNING_POP
     \sa setValue(), value()
 */
 
+/*! \fn template<typename T> static QVariant QVariant::fromValue(T &&value)
+
+    \since 6.6
+    \overload
+*/
+
 /*! \fn template<typename... Types> QVariant QVariant::fromStdVariant(const std::variant<Types...> &value)
     \since 5.11
 

src/corelib/kernel/qvariant.h

@@ -71,6 +71,9 @@ class Q_CORE_EXPORT QVariant
         >,
     bool>;
 
+    template <typename T>
+    using if_rvalue = std::enable_if_t<!std::is_reference_v<T>, bool>;
+
     struct CborValueStandIn { qint64 n; void *c; int t; };
 public:
     struct PrivateShared
@@ -516,6 +519,39 @@ public:
         return t;
     }
 
+    template<typename T, if_rvalue<T> = true>
+#ifndef Q_QDOC
+        /* needs is_copy_constructible for variants semantics, is_move_constructible so that moveConstruct works
+          (but copy_constructible implies move_constructble, so don't bother checking)
+        */
+    static inline auto fromValue(T &&value)
+        noexcept(std::is_nothrow_copy_constructible_v<T> && Private::CanUseInternalSpace<T>)
+        -> std::enable_if_t<std::conjunction_v<std::is_copy_constructible<T>,
+                                               std::is_destructible<T>>, QVariant>
+#else
+    static inline QVariant fromValue(T &&value)
+#endif
+    {
+        // handle special cases
+        using Type = std::remove_cv_t<T>;
+        if constexpr (std::is_null_pointer_v<Type>)
+            return QVariant(QMetaType::fromType<std::nullptr_t>());
+        else if constexpr (std::is_same_v<Type, QVariant>)
+            return std::forward<T>(value);
+        else if constexpr (std::is_same_v<Type, std::monostate>)
+            return QVariant();
+        QMetaType mt = QMetaType::fromType<Type>();
+        mt.registerType(); // we want the type stored in QVariant to always be registered
+        // T is a forwarding reference, so if T satifies the enable-ifery,
+        // we get this overload even if T is an lvalue reference and thus must check here
+        // Moreover, we only try to move if the type is actually moveable and not if T is const
+        // as in const int i; QVariant::fromValue(std::move(i));
+        if constexpr (std::conjunction_v<std::is_move_constructible<Type>, std::negation<std::is_const<T>>>)
+            return moveConstruct(QMetaType::fromType<Type>(), std::addressof(value));
+        else
+            return copyConstruct(mt, std::addressof(value));
+    }
+
     template<typename T>
 #ifndef Q_QDOC
     static inline auto fromValue(const T &value)
@@ -594,6 +630,9 @@ private:
     Q_MK_GET(const &&)
 #undef Q_MK_GET
 
+    static QVariant moveConstruct(QMetaType type, void *data);
+    static QVariant copyConstruct(QMetaType type, const void *data);
+
     template<typename T>
     friend inline T qvariant_cast(const QVariant &);
 protected:

tests/auto/corelib/kernel/qvariant/tst_qvariant.cpp

@@ -5573,6 +5573,16 @@ void tst_QVariant::mutableView()
     QCOMPARE(extracted.text, nullptr);
 }
 
+struct MoveTester
+{
+    bool wasMoved = false;
+    MoveTester() = default;
+    MoveTester(const MoveTester &) {}; // non-trivial on purpose
+    MoveTester(MoveTester &&other) { other.wasMoved = true; }
+    MoveTester& operator=(const MoveTester &) = default;
+    MoveTester& operator=(MoveTester &&other) {other.wasMoved = true; return *this;}
+};
+
 void tst_QVariant::moveOperations()
 {
     {
@@ -5594,6 +5604,19 @@ void tst_QVariant::moveOperations()
     v = QVariant::fromValue(list);
     v2 = std::move(v);
     QVERIFY(v2.value<std::list<int>>() == list);
+
+    {
+        MoveTester tester;
+        QVariant::fromValue(tester);
+        QVERIFY(!tester.wasMoved);
+        QVariant::fromValue(std::move(tester));
+        QVERIFY(tester.wasMoved);
+    }
+    {
+        const MoveTester tester;
+        QVariant::fromValue(std::move(tester));
+        QVERIFY(!tester.wasMoved); // we don't want to move from const variables
+    }
 }
 
 class NoMetaObject : public QObject {};