QtPositioning: refactor QGeoSatelliteInfo

The following changes were implemented for the class:
- Use QExplicitlySharedDataPointer for private d-ptr.
- Implement move-constructor and move-assignment operator.
  The moved-from object is left in partially-formed state. Such
  behavior is documented.
- Use Q_DECLARE_SHARED to declare the typeinfo as Q_RELOCATABLE_TYPE
  and provide a free swap() overload.
- Provide a qHash() overload.
- Provide a QTest::toString() overload.

All these changes are common pattern for Qt value classes.
The benchmark shows that it results in slight performance drop while
constructing a brand new object (most probably because of the overhead
of QSharedData and QExplicitlySharedDataPointer), but also gives a
comparable performance boost for copy constructor and copy assignment.
And of course a significant performance boost on move-operations (which
were just copy-operations previously). Other API calls didn't have any
significant performance impact.

Task-number: QTBUG-90491
Change-Id: I51ed7041c4b9ecd763f045abbb80df5e0dcde8f6
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

6 files changed, 132 insertions, 28 deletions

diff --git a/src/plugins/position/android/src/qgeosatelliteinfosource_android.cpp b/src/plugins/position/android/src/qgeosatelliteinfosource_android.cpp
index efa634ee..13c09680 100644
--- a/src/plugins/position/android/src/qgeosatelliteinfosource_android.cpp
+++ b/src/plugins/position/android/src/qgeosatelliteinfosource_android.cpp
@@ -42,7 +42,6 @@
 #include "qgeosatelliteinfosource_android_p.h"
 #include "jnipositioning.h"
 
-Q_DECLARE_METATYPE(QGeoSatelliteInfo)
 Q_DECLARE_METATYPE(QList<QGeoSatelliteInfo>)
 
 #define UPDATE_FROM_COLD_START 2*60*1000
diff --git a/src/plugins/position/serialnmea/qnmeasatelliteinfosource.cpp b/src/plugins/position/serialnmea/qnmeasatelliteinfosource.cpp
index e08bc5e2..a2cd5447 100644
--- a/src/plugins/position/serialnmea/qnmeasatelliteinfosource.cpp
+++ b/src/plugins/position/serialnmea/qnmeasatelliteinfosource.cpp
@@ -62,7 +62,6 @@ public:
     QGeoSatelliteInfoPrivateNmea(const QGeoSatelliteInfoPrivate &other);
     QGeoSatelliteInfoPrivateNmea(const QGeoSatelliteInfoPrivateNmea &other);
     virtual ~QGeoSatelliteInfoPrivateNmea();
-    QGeoSatelliteInfoPrivate *clone() const override;
 
     QList<QByteArray> nmeaSentences;
 };
@@ -79,11 +78,6 @@ QGeoSatelliteInfoPrivateNmea::QGeoSatelliteInfoPrivateNmea(const QGeoSatelliteIn
 }
 
 QGeoSatelliteInfoPrivateNmea::~QGeoSatelliteInfoPrivateNmea() {}
-
-QGeoSatelliteInfoPrivate *QGeoSatelliteInfoPrivateNmea::clone() const
-{
-    return new QGeoSatelliteInfoPrivateNmea(*this);
-}
 #else
 typedef QGeoSatelliteInfoPrivate QGeoSatelliteInfoPrivateNmea;
 #endif
diff --git a/src/positioning/qgeosatelliteinfo.cpp b/src/positioning/qgeosatelliteinfo.cpp
index 91ebfa85..3b73dfed 100644
--- a/src/positioning/qgeosatelliteinfo.cpp
+++ b/src/positioning/qgeosatelliteinfo.cpp
@@ -37,7 +37,7 @@
 **
 ****************************************************************************/
 #include "qgeosatelliteinfo.h"
-#include "qgeosatelliteinfo_p.h"
+#include "private/qgeosatelliteinfo_p.h"
 
 #include <QHash>
 #include <QDebug>
@@ -49,6 +49,7 @@ QT_BEGIN_NAMESPACE
     \class QGeoSatelliteInfo
     \inmodule QtPositioning
     \ingroup QtPositioning-positioning
+    \ingroup shared
     \since 5.2
 
     \brief The QGeoSatelliteInfo class contains basic information about a satellite.
@@ -89,9 +90,8 @@ QGeoSatelliteInfo::QGeoSatelliteInfo()
 */
 
 QGeoSatelliteInfo::QGeoSatelliteInfo(const QGeoSatelliteInfo &other)
-        : d(new QGeoSatelliteInfoPrivate)
+        : d(other.d)
 {
-    operator=(other);
 }
 
 QGeoSatelliteInfo::QGeoSatelliteInfo(QGeoSatelliteInfoPrivate &dd) : d(&dd)
@@ -99,13 +99,25 @@ QGeoSatelliteInfo::QGeoSatelliteInfo(QGeoSatelliteInfoPrivate &dd) : d(&dd)
 }
 
 /*!
+    \fn QGeoSatelliteInfo::QGeoSatelliteInfo(QGeoSatelliteInfo &&other) noexcept
+    \since 6.2
+
+    Creates a satellite information object by moving from \a other.
+
+    Note that a moved-from QGeoSatelliteInfo can only be destroyed or
+    assigned to. The effect of calling other functions than the destructor
+    or one of the assignment operators is undefined.
+*/
+
+/*!
     Destroys a satellite information object.
 */
 QGeoSatelliteInfo::~QGeoSatelliteInfo()
 {
-    delete d;
 }
 
+QT_DEFINE_QESDP_SPECIALIZATION_DTOR(QGeoSatelliteInfoPrivate)
+
 /*!
     Assigns the values from \a other to this object.
 */
@@ -114,13 +126,22 @@ QGeoSatelliteInfo &QGeoSatelliteInfo::operator=(const QGeoSatelliteInfo & other)
     if (this == &other)
         return *this;
 
-    delete d;
-    d = other.d->clone();
-
+    d = other.d;
     return *this;
 }
 
 /*!
+    \fn QGeoSatelliteInfo &QGeoSatelliteInfo::operator=(QGeoSatelliteInfo &&other) noexcept
+    \since 6.2
+
+    Move-assigns the value from \a other to this object
+
+    Note that a moved-from QGeoSatelliteInfo can only be destroyed or
+    assigned to. The effect of calling other functions than the destructor
+    or one of the assignment operators is undefined.
+*/
+
+/*!
     Returns true if all the information for this satellite
     are the same as those of \a other.
 */
@@ -142,6 +163,7 @@ bool QGeoSatelliteInfo::operator==(const QGeoSatelliteInfo &other) const
 */
 void QGeoSatelliteInfo::setSatelliteSystem(SatelliteSystem system)
 {
+    d.detach();
     d->system = system;
 }
 
@@ -162,6 +184,7 @@ QGeoSatelliteInfo::SatelliteSystem QGeoSatelliteInfo::satelliteSystem() const
 */
 void QGeoSatelliteInfo::setSatelliteIdentifier(int satId)
 {
+    d.detach();
     d->satId = satId;
 }
 
@@ -182,6 +205,7 @@ int QGeoSatelliteInfo::satelliteIdentifier() const
 */
 void QGeoSatelliteInfo::setSignalStrength(int signalStrength)
 {
+    d.detach();
     d->signal = signalStrength;
 }
 
@@ -198,6 +222,7 @@ int QGeoSatelliteInfo::signalStrength() const
 */
 void QGeoSatelliteInfo::setAttribute(Attribute attribute, qreal value)
 {
+    d.detach();
     d->doubleAttribs[int(attribute)] = value;
 }
 
@@ -220,6 +245,7 @@ qreal QGeoSatelliteInfo::attribute(Attribute attribute) const
 */
 void QGeoSatelliteInfo::removeAttribute(Attribute attribute)
 {
+    d.detach();
     d->doubleAttribs.remove(int(attribute));
 }
 
@@ -231,6 +257,17 @@ bool QGeoSatelliteInfo::hasAttribute(Attribute attribute) const
     return d->doubleAttribs.contains(int(attribute));
 }
 
+/*!
+    \internal
+*/
+void QGeoSatelliteInfo::detach()
+{
+    if (d)
+        d.detach();
+    else
+        d = new QGeoSatelliteInfoPrivate;
+}
+
 #ifndef QT_NO_DEBUG_STREAM
 QDebug operator<<(QDebug dbg, const QGeoSatelliteInfo &info)
 {
@@ -256,7 +293,7 @@ QDebug operator<<(QDebug dbg, const QGeoSatelliteInfo &info)
     dbg << ')';
     return dbg;
 }
-#endif
+#endif // QT_NO_DEBUG_STREAM
 
 #ifndef QT_NO_DATASTREAM
 /*!
@@ -277,7 +314,7 @@ QDataStream &operator<<(QDataStream &stream, const QGeoSatelliteInfo &info)
     stream << int(info.d->system);
     return stream;
 }
-#endif
+#endif // QT_NO_DATASTREAM
 
 #ifndef QT_NO_DATASTREAM
 /*!
@@ -300,13 +337,15 @@ QDataStream &operator>>(QDataStream &stream, QGeoSatelliteInfo &info)
     info.d->system = (QGeoSatelliteInfo::SatelliteSystem)system;
     return stream;
 }
+#endif // QT_NO_DATASTREAM
 
-QGeoSatelliteInfoPrivate::QGeoSatelliteInfoPrivate()
+QGeoSatelliteInfoPrivate::QGeoSatelliteInfoPrivate() : QSharedData()
 {
 
 }
 
 QGeoSatelliteInfoPrivate::QGeoSatelliteInfoPrivate(const QGeoSatelliteInfoPrivate &other)
+    : QSharedData(other)
 {
     signal = other.signal;
     satId = other.satId;
@@ -316,11 +355,6 @@ QGeoSatelliteInfoPrivate::QGeoSatelliteInfoPrivate(const QGeoSatelliteInfoPrivat
 
 QGeoSatelliteInfoPrivate::~QGeoSatelliteInfoPrivate() {}
 
-QGeoSatelliteInfoPrivate *QGeoSatelliteInfoPrivate::clone() const
-{
-    return new QGeoSatelliteInfoPrivate(*this);
-}
-
 bool QGeoSatelliteInfoPrivate::operator==(const QGeoSatelliteInfoPrivate &other) const
 {
     return signal == other.signal
@@ -331,9 +365,28 @@ bool QGeoSatelliteInfoPrivate::operator==(const QGeoSatelliteInfoPrivate &other)
 
 QGeoSatelliteInfoPrivate *QGeoSatelliteInfoPrivate::get(const QGeoSatelliteInfo &info)
 {
-    return info.d;
+    return info.d.data();
+}
+
+size_t qHash(const QGeoSatelliteInfo &key, size_t seed) noexcept
+{
+    // Other properties and attributes might change
+    return qHashMulti(seed, key.d->satId, key.d->system);
+}
+
+namespace QTest
+{
+
+char *toString(const QGeoSatelliteInfo &info)
+{
+    QString result;
+    QDebug dbg(&result);
+    dbg << info;
+
+    return qstrdup(qPrintable(result));
+}
+
 }
 
-#endif
 
 QT_END_NAMESPACE
diff --git a/src/positioning/qgeosatelliteinfo.h b/src/positioning/qgeosatelliteinfo.h
index 28766257..ee183b30 100644
--- a/src/positioning/qgeosatelliteinfo.h
+++ b/src/positioning/qgeosatelliteinfo.h
@@ -40,13 +40,26 @@
 #define QGEOSATELLITEINFO_H
 
 #include <QtPositioning/qpositioningglobal.h>
+#include <QtCore/QSharedData>
+#include <QtCore/QMetaType>
 
 QT_BEGIN_NAMESPACE
 
 class QDebug;
 class QDataStream;
 
+class QGeoSatelliteInfo;
+Q_POSITIONING_EXPORT size_t qHash(const QGeoSatelliteInfo &key, size_t seed = 0) noexcept;
+namespace QTest
+{
+
+Q_POSITIONING_EXPORT char *toString(const QGeoSatelliteInfo &info);
+
+} // namespace QTest
+
 class QGeoSatelliteInfoPrivate;
+QT_DECLARE_QESDP_SPECIALIZATION_DTOR_WITH_EXPORT(QGeoSatelliteInfoPrivate, Q_POSITIONING_EXPORT)
+
 class Q_POSITIONING_EXPORT QGeoSatelliteInfo
 {
 public:
@@ -64,9 +77,13 @@ public:
     QGeoSatelliteInfo();
     QGeoSatelliteInfo(const QGeoSatelliteInfo &other);
     QGeoSatelliteInfo(QGeoSatelliteInfoPrivate &dd);
+    QGeoSatelliteInfo(QGeoSatelliteInfo &&other) noexcept = default;
     ~QGeoSatelliteInfo();
 
     QGeoSatelliteInfo &operator=(const QGeoSatelliteInfo &other);
+    QT_MOVE_ASSIGNMENT_OPERATOR_IMPL_VIA_PURE_SWAP(QGeoSatelliteInfo)
+
+    void swap(QGeoSatelliteInfo &other) noexcept { d.swap(other.d); }
 
     bool operator==(const QGeoSatelliteInfo &other) const;
     inline bool operator!=(const QGeoSatelliteInfo &other) const {
@@ -88,6 +105,8 @@ public:
 
     bool hasAttribute(Attribute attribute) const;
 
+    void detach();
+
 private:
 #ifndef QT_NO_DEBUG_STREAM
     friend Q_POSITIONING_EXPORT QDebug operator<<(QDebug dbg, const QGeoSatelliteInfo &info);
@@ -96,10 +115,15 @@ private:
     friend Q_POSITIONING_EXPORT QDataStream &operator<<(QDataStream &stream, const QGeoSatelliteInfo &info);
     friend Q_POSITIONING_EXPORT QDataStream &operator>>(QDataStream &stream, QGeoSatelliteInfo &info);
 #endif
-    QGeoSatelliteInfoPrivate *d;
+    QExplicitlySharedDataPointer<QGeoSatelliteInfoPrivate> d;
     friend class QGeoSatelliteInfoPrivate;
+
+    friend Q_POSITIONING_EXPORT size_t qHash(const QGeoSatelliteInfo &key, size_t seed) noexcept;
+    friend Q_POSITIONING_EXPORT char *QTest::toString(const QGeoSatelliteInfo &info);
 };
 
+Q_DECLARE_SHARED(QGeoSatelliteInfo)
+
 #ifndef QT_NO_DEBUG_STREAM
 Q_POSITIONING_EXPORT QDebug operator<<(QDebug dbg, const QGeoSatelliteInfo &info);
 #endif
@@ -111,4 +135,6 @@ Q_POSITIONING_EXPORT QDataStream &operator>>(QDataStream &stream, QGeoSatelliteI
 
 QT_END_NAMESPACE
 
+Q_DECLARE_METATYPE(QGeoSatelliteInfo)
+
 #endif
diff --git a/src/positioning/qgeosatelliteinfo_p.h b/src/positioning/qgeosatelliteinfo_p.h
index 2818f781..ff354304 100644
--- a/src/positioning/qgeosatelliteinfo_p.h
+++ b/src/positioning/qgeosatelliteinfo_p.h
@@ -57,13 +57,12 @@
 
 QT_BEGIN_NAMESPACE
 
-class Q_POSITIONING_PRIVATE_EXPORT QGeoSatelliteInfoPrivate
+class Q_POSITIONING_PRIVATE_EXPORT QGeoSatelliteInfoPrivate : public QSharedData
 {
 public:
     QGeoSatelliteInfoPrivate();
     QGeoSatelliteInfoPrivate(const QGeoSatelliteInfoPrivate &other);
     virtual ~QGeoSatelliteInfoPrivate();
-    virtual QGeoSatelliteInfoPrivate *clone() const;
     virtual bool operator==(const QGeoSatelliteInfoPrivate &other) const;
     static QGeoSatelliteInfoPrivate *get(const QGeoSatelliteInfo &info);
 
diff --git a/tests/auto/qgeosatelliteinfo/tst_qgeosatelliteinfo.cpp b/tests/auto/qgeosatelliteinfo/tst_qgeosatelliteinfo.cpp
index e1b0ad46..c99e222f 100644
--- a/tests/auto/qgeosatelliteinfo/tst_qgeosatelliteinfo.cpp
+++ b/tests/auto/qgeosatelliteinfo/tst_qgeosatelliteinfo.cpp
@@ -39,7 +39,6 @@
 #include <limits.h>
 
 QT_USE_NAMESPACE
-Q_DECLARE_METATYPE(QGeoSatelliteInfo)
 Q_DECLARE_METATYPE(QGeoSatelliteInfo::Attribute)
 
 QByteArray tst_qgeosatelliteinfo_debug;
@@ -156,6 +155,21 @@ private slots:
         addTestData_update();
     }
 
+    void constructor_move()
+    {
+        QFETCH(QGeoSatelliteInfo, info);
+        QGeoSatelliteInfo infoCopy = info;
+        QCOMPARE(QGeoSatelliteInfo(std::move(info)), infoCopy);
+        // The moved-from object will go out of scope and  will be destroyed
+        // here, so we also implicitly check that moved-from object's destructor
+        // is called without any issues.
+    }
+
+    void constructor_move_data()
+    {
+        addTestData_update();
+    }
+
     void operator_comparison()
     {
         QFETCH(QGeoSatelliteInfo, info);
@@ -186,6 +200,25 @@ private slots:
         addTestData_update();
     }
 
+    void operator_move_assign()
+    {
+        QFETCH(QGeoSatelliteInfo, info);
+        QGeoSatelliteInfo infoCopy = info;
+
+        QGeoSatelliteInfo obj;
+        obj = std::move(info);
+        QCOMPARE(obj, infoCopy);
+
+        // check that (move)assigning to the moved-from object is ok
+        info = std::move(infoCopy);
+        QCOMPARE(info, obj);
+    }
+
+    void operator_move_assign_data()
+    {
+        addTestData_update();
+    }
+
     void setSignalStrength()
     {
         QFETCH(int, signal);