QExplicitlySharedDataPointer: Use new comparison helper macros

Provide the new comparesEqual() helper function as an implementation of
the (in)equality operators and compareThreeWay() helper function for
the rest of the relational operators.
Use Q_DECLARE_STRONGLY_ORDERED to provide all relational operators.

Use the new Qt::totally_ordered_wrapper to wrap the "d" pointer to
avoid UB when performing comparisons of QExplicitlySharedDataPointer.

Add some comparisons related tests for QExplicitlySharedDataPointer.
Use QT_TEST_ALL_COMPARISON_OPS macros in unit-tests.

Task-number: QTBUG-120306
Change-Id: I275484a96a57da2df92712ac97e237a88a889da5
Reviewed-by: Ivan Solovev <ivan.solovev@qt.io>

src/corelib/tools/qshareddata.cpp

@@ -443,6 +443,10 @@ QT_BEGIN_NAMESPACE
     \since 4.4
     \reentrant
 
+    \compares strong
+    \compareswith strong T* std::nullptr_t
+    \endcompareswith
+
     QExplicitlySharedDataPointer\<T\> makes writing your own explicitly
     shared classes easy. QExplicitlySharedDataPointer implements
     \l {thread-safe} reference counting, ensuring that adding
@@ -520,8 +524,8 @@ QT_BEGIN_NAMESPACE
     \since 5.2
 */
 
-/*! \fn template <class T> bool QExplicitlySharedDataPointer<T>::operator==(const T* ptr, const QExplicitlySharedDataPointer<T>& rhs)
+/*! \fn template <class T> bool QExplicitlySharedDataPointer<T>::operator==(const T* const &lhs, const QExplicitlySharedDataPointer<T>& rhs)
-    Returns \c true if the \e{d pointer} of \a rhs is \a ptr.
+    Returns \c true if the \e{d pointer} of \a rhs is \a lhs.
  */
 
 /*! \fn template <class T> bool QExplicitlySharedDataPointer<T>::operator!=(const QExplicitlySharedDataPointer<T>& lhs, const QExplicitlySharedDataPointer<T>& rhs)
@@ -529,8 +533,8 @@ QT_BEGIN_NAMESPACE
     \e{d pointer}.
 */
 
-/*! \fn template <class T> bool QExplicitlySharedDataPointer<T>::operator!=(const T* ptr, const QExplicitlySharedDataPointer<T>& rhs)
+/*! \fn template <class T> bool QExplicitlySharedDataPointer<T>::operator!=(const T* const &lhs, const QExplicitlySharedDataPointer<T>& rhs)
-    Returns \c true if the \e{d pointer} of \a rhs is \e not \a ptr.
+    Returns \c true if the \e{d pointer} of \a rhs is \e not \a lhs.
  */
 
 /*! \fn template <class T> QExplicitlySharedDataPointer<T>::QExplicitlySharedDataPointer()

src/corelib/tools/qshareddata.h

@@ -134,21 +134,21 @@ public:
     typedef T Type;
     typedef T *pointer;
 
-    T &operator*() const { return *d; }
+    T &operator*() const { return *(d.get()); }
-    T *operator->() noexcept { return d; }
+    T *operator->() noexcept { return d.get(); }
-    T *operator->() const noexcept { return d; }
+    T *operator->() const noexcept { return d.get(); }
-    explicit operator T *() { return d; }
+    explicit operator T *() { return d.get(); }
-    explicit operator const T *() const noexcept { return d; }
+    explicit operator const T *() const noexcept { return d.get(); }
-    T *data() const noexcept { return d; }
+    T *data() const noexcept { return d.get(); }
-    T *get() const noexcept { return d; }
+    T *get() const noexcept { return d.get(); }
-    const T *constData() const noexcept { return d; }
+    const T *constData() const noexcept { return d.get(); }
-    T *take() noexcept { return std::exchange(d, nullptr); }
+    T *take() noexcept { return std::exchange(d, nullptr).get(); }
 
     void detach() { if (d && d->ref.loadRelaxed() != 1) detach_helper(); }
 
     Q_NODISCARD_CTOR
     QExplicitlySharedDataPointer() noexcept : d(nullptr) { }
-    ~QExplicitlySharedDataPointer() { if (d && !d->ref.deref()) delete d; }
+    ~QExplicitlySharedDataPointer() { if (d && !d->ref.deref()) delete d.get(); }
 
     Q_NODISCARD_CTOR
     explicit QExplicitlySharedDataPointer(T *data) noexcept : d(data)
@@ -174,7 +174,7 @@ public:
         if (ptr != d) {
             if (ptr)
                 ptr->ref.ref();
-            T *old = std::exchange(d, ptr);
+            T *old = std::exchange(d, Qt::totally_ordered_wrapper(ptr)).get();
             if (old && !old->ref.deref())
                 delete old;
         }
@@ -182,7 +182,7 @@ public:
 
     QExplicitlySharedDataPointer &operator=(const QExplicitlySharedDataPointer &o) noexcept
     {
-        reset(o.d);
+        reset(o.d.get());
         return *this;
     }
     QExplicitlySharedDataPointer &operator=(T *o) noexcept
@@ -200,35 +200,36 @@ public:
     void swap(QExplicitlySharedDataPointer &other) noexcept
     { qt_ptr_swap(d, other.d); }
 
-#define DECLARE_COMPARE_SET(T1, A1, T2, A2) \
-    friend bool operator<(T1, T2) noexcept \
-    { return std::less<T*>{}(A1, A2); } \
-    friend bool operator<=(T1, T2) noexcept \
-    { return !std::less<T*>{}(A2, A1); } \
-    friend bool operator>(T1, T2) noexcept \
-    { return std::less<T*>{}(A2, A1); } \
-    friend bool operator>=(T1, T2) noexcept \
-    { return !std::less<T*>{}(A1, A2); } \
-    friend bool operator==(T1, T2) noexcept \
-    { return A1 == A2; } \
-    friend bool operator!=(T1, T2) noexcept \
-    { return A1 != A2; } \
-
-    DECLARE_COMPARE_SET(const QExplicitlySharedDataPointer &p1, p1.d, const QExplicitlySharedDataPointer &p2, p2.d)
-    DECLARE_COMPARE_SET(const QExplicitlySharedDataPointer &p1, p1.d, const T *ptr, ptr)
-    DECLARE_COMPARE_SET(const T *ptr, ptr, const QExplicitlySharedDataPointer &p2, p2.d)
-    DECLARE_COMPARE_SET(const QExplicitlySharedDataPointer &p1, p1.d, std::nullptr_t, nullptr)
-    DECLARE_COMPARE_SET(std::nullptr_t, nullptr, const QExplicitlySharedDataPointer &p2, p2.d)
-
-#undef DECLARE_COMPARE_SET
-
 protected:
     T *clone();
 
 private:
+    friend bool comparesEqual(const QExplicitlySharedDataPointer &lhs,
+                              const QExplicitlySharedDataPointer &rhs) noexcept
+    { return lhs.d == rhs.d; }
+    friend Qt::strong_ordering
+    compareThreeWay(const QExplicitlySharedDataPointer &lhs,
+                    const QExplicitlySharedDataPointer &rhs) noexcept
+    { return Qt::compareThreeWay(lhs.d, rhs.d); }
+    Q_DECLARE_STRONGLY_ORDERED(QExplicitlySharedDataPointer)
+
+    friend bool comparesEqual(const QExplicitlySharedDataPointer &lhs, const T *rhs) noexcept
+    { return lhs.d == rhs; }
+    friend Qt::strong_ordering
+    compareThreeWay(const QExplicitlySharedDataPointer &lhs, const T *rhs) noexcept
+    { return Qt::compareThreeWay(lhs.d, rhs); }
+    Q_DECLARE_STRONGLY_ORDERED(QExplicitlySharedDataPointer, const T*)
+
+    friend bool comparesEqual(const QExplicitlySharedDataPointer &lhs, std::nullptr_t) noexcept
+    { return lhs.d == nullptr; }
+    friend Qt::strong_ordering
+    compareThreeWay(const QExplicitlySharedDataPointer &lhs, std::nullptr_t) noexcept
+    { return Qt::compareThreeWay(lhs.d, nullptr); }
+    Q_DECLARE_STRONGLY_ORDERED(QExplicitlySharedDataPointer, std::nullptr_t)
+
     void detach_helper();
 
-    T *d;
+    Qt::totally_ordered_wrapper<T *> d;
 };
 
 // Declared here and as Q_OUTOFLINE_TEMPLATE to work-around MSVC bug causing missing symbols at link time.
@@ -251,7 +252,7 @@ Q_OUTOFLINE_TEMPLATE void QSharedDataPointer<T>::detach_helper()
 template <typename T>
 Q_INLINE_TEMPLATE T *QExplicitlySharedDataPointer<T>::clone()
 {
-    return new T(*d);
+    return new T(*d.get());
 }
 
 template <typename T>
@@ -260,8 +261,8 @@ Q_OUTOFLINE_TEMPLATE void QExplicitlySharedDataPointer<T>::detach_helper()
     T *x = clone();
     x->ref.ref();
     if (!d->ref.deref())
-        delete d;
+        delete d.get();
-    d = x;
+    d.reset(x);
 }
 
 template <typename T>
@@ -309,7 +310,7 @@ template<typename T> Q_DECLARE_TYPEINFO_BODY(QExplicitlySharedDataPointer<T>, Q_
     template<> QExplicitlySharedDataPointer<Class>::~QExplicitlySharedDataPointer() \
     { \
         if (d && !d->ref.deref()) \
-            delete d; \
+            delete d.get(); \
     }
 
 QT_END_NAMESPACE

src/dbus/qdbusunixfiledescriptor.cpp

@@ -89,9 +89,7 @@ public:
     QAtomicInt fd;
 };
 
-template<> inline
+QT_DEFINE_QESDP_SPECIALIZATION_DTOR(QDBusUnixFileDescriptorPrivate)
-QExplicitlySharedDataPointer<QDBusUnixFileDescriptorPrivate>::~QExplicitlySharedDataPointer()
-{ if (d && !d->ref.deref()) delete d; }
 
 /*!
     Constructs a QDBusUnixFileDescriptor without a wrapped file descriptor.

src/network/access/qhttpheaders.cpp

@@ -802,7 +802,7 @@ QT_DEFINE_QESDP_SPECIALIZATION_DTOR(QHttpHeadersPrivate)
 template <> void QExplicitlySharedDataPointer<QHttpHeadersPrivate>::detach()
 {
     if (!d) {
-        d = new QHttpHeadersPrivate();
+        d.reset(new QHttpHeadersPrivate());
         d->ref.ref();
     } else if (d->ref.loadRelaxed() != 1) {
         detach_helper();

tests/auto/corelib/tools/qexplicitlyshareddatapointer/CMakeLists.txt

@@ -14,4 +14,6 @@ endif()
 qt_internal_add_test(tst_qexplicitlyshareddatapointer
     SOURCES
         tst_qexplicitlyshareddatapointer.cpp
+    LIBRARIES
+        Qt::TestPrivate
 )

tests/auto/corelib/tools/qexplicitlyshareddatapointer/tst_qexplicitlyshareddatapointer.cpp

@@ -3,6 +3,8 @@
 
 
 #include <QTest>
+#include <QtTest/private/qcomparisontesthelper_p.h>
+
 #include <QtCore/QSharedData>
 
 /*!
@@ -24,11 +26,19 @@ private Q_SLOTS:
     void data() const;
     void reset() const;
     void swap() const;
+    void compareCompiles() const;
+    void compare() const;
 };
 
 class MyClass : public QSharedData
 {
 public:
+    MyClass() = default;
+    MyClass(int v) : m_value(v)
+    {
+        ref.ref();
+    };
+    int m_value;
     void mutating()
     {
     }
@@ -220,6 +230,41 @@ void tst_QExplicitlySharedDataPointer::swap() const
     QVERIFY(!p2.data());
 }
 
+void tst_QExplicitlySharedDataPointer::compareCompiles() const
+{
+    QTestPrivate::testAllComparisonOperatorsCompile<QExplicitlySharedDataPointer<MyClass>>();
+    QTestPrivate::testAllComparisonOperatorsCompile<QExplicitlySharedDataPointer<MyClass>,
+                                                    MyClass*>();
+    QTestPrivate::testAllComparisonOperatorsCompile<QExplicitlySharedDataPointer<MyClass>,
+                                                    std::nullptr_t>();
+}
+
+void tst_QExplicitlySharedDataPointer::compare() const
+{
+    const QExplicitlySharedDataPointer<MyClass> ptr;
+    const QExplicitlySharedDataPointer<MyClass> ptr2;
+    QT_TEST_ALL_COMPARISON_OPS(ptr, nullptr, Qt::strong_ordering::equal);
+    QT_TEST_ALL_COMPARISON_OPS(ptr2, nullptr, Qt::strong_ordering::equal);
+    QT_TEST_ALL_COMPARISON_OPS(ptr, ptr2, Qt::strong_ordering::equal);
+
+    const QExplicitlySharedDataPointer<MyClass> copy(ptr);
+    QT_TEST_ALL_COMPARISON_OPS(ptr, copy, Qt::strong_ordering::equal);
+
+    MyClass* pointer = nullptr;
+    QT_TEST_ALL_COMPARISON_OPS(pointer, ptr, Qt::strong_ordering::equal);
+
+    const QExplicitlySharedDataPointer<MyClass> pointer2(new MyClass());
+    QT_TEST_ALL_COMPARISON_OPS(pointer, pointer2, Qt::strong_ordering::less);
+
+    std::array<MyClass, 3> myArray {MyClass(2), MyClass(1), MyClass(0)};
+    const QExplicitlySharedDataPointer<const MyClass> val0(&myArray[0]);
+    const QExplicitlySharedDataPointer<const MyClass> val1(&myArray[1]);
+    QT_TEST_ALL_COMPARISON_OPS(val0, val1, Qt::strong_ordering::less);
+    QT_TEST_ALL_COMPARISON_OPS(val0, &myArray[1], Qt::strong_ordering::less);
+    QT_TEST_ALL_COMPARISON_OPS(val1, val0, Qt::strong_ordering::greater);
+    QT_TEST_ALL_COMPARISON_OPS(&myArray[1], val0, Qt::strong_ordering::greater);
+}
+
 QTEST_MAIN(tst_QExplicitlySharedDataPointer)
 
 #include "tst_qexplicitlyshareddatapointer.moc"