From 7131240754d4caa3891f667b4645d8915cb82535 Mon Sep 17 00:00:00 2001 From: Rym Bouabid Date: Wed, 3 Apr 2024 16:56:34 +0200 Subject: [PATCH] QSharedDataPointer: Use new comparison helper macros MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 QSharedDataPointer. The old implementation of the operators between QSharedDataPointer and the normal pointer T* used to cause a bug but this was not noticed because QSharedDataPointer didn't have any tests. I added tests and tried to compare using operator<(). I got build issues: error: no match for call to ‘(std::less) (const MyClass*&, MyClass* const&)’ This happens because QSharedDataPointer has an implicit conversion to MyClass*. Making the normal pointer T* non-const as follow resolves the issue: DECLARE_COMPARE_SET(const QSharedDataPointer &p1, p1.d, T *ptr, ptr) DECLARE_COMPARE_SET(T *ptr, ptr, const QSharedDataPointer &p2, p2.d) A similar bug happens if we make the normal pointer const in the comparison macro Q_DECLARE_STRONGLY_ORDERED. The relational operators will be ambiguous. The compiler picks between operator<(QSharedDataPointer, MyClass*) and opeartor<(MyClass*, MyClass*) (In case we're calling <). Therefore, use: Q_DECLARE_STRONGLY_ORDERED(QSharedDataPointer, T*) Add some comparisons related tests for QSharedDataPointer. Use QT_TEST_ALL_COMPARISON_OPS macros in unit-tests. Task-number: QTBUG-120306 Change-Id: I8d1159a01651bab442b79a89c53e064914103133 Reviewed-by: Ivan Solovev --- src/corelib/io/qprocess_p.h | 4 +- src/corelib/io/qurlquery.cpp | 4 +- src/corelib/tools/qshareddata.cpp | 12 ++- src/corelib/tools/qshareddata.h | 98 +++++++++++-------- src/network/kernel/qnetworkproxy.cpp | 8 +- tests/auto/corelib/tools/CMakeLists.txt | 1 + .../tools/qshareddatapointer/CMakeLists.txt | 19 ++++ .../tst_qshareddatapointer.cpp | 63 ++++++++++++ 8 files changed, 156 insertions(+), 53 deletions(-) create mode 100644 tests/auto/corelib/tools/qshareddatapointer/CMakeLists.txt create mode 100644 tests/auto/corelib/tools/qshareddatapointer/tst_qshareddatapointer.cpp diff --git a/src/corelib/io/qprocess_p.h b/src/corelib/io/qprocess_p.h index 9510101e745..77ebe5ac2b0 100644 --- a/src/corelib/io/qprocess_p.h +++ b/src/corelib/io/qprocess_p.h @@ -182,8 +182,8 @@ template<> Q_INLINE_TEMPLATE void QSharedDataPointer : new QProcessEnvironmentPrivate); x->ref.ref(); if (d && !d->ref.deref()) - delete d; - d = x; + delete d.get(); + d.reset(x); } #if QT_CONFIG(process) diff --git a/src/corelib/io/qurlquery.cpp b/src/corelib/io/qurlquery.cpp index 31f3ee1d902..c155451edc3 100644 --- a/src/corelib/io/qurlquery.cpp +++ b/src/corelib/io/qurlquery.cpp @@ -174,8 +174,8 @@ template<> void QSharedDataPointer::detach() : new QUrlQueryPrivate); x->ref.ref(); if (d && !d->ref.deref()) - delete d; - d = x; + delete d.get(); + d.reset(x); } // Here's how we do the encoding in QUrlQuery diff --git a/src/corelib/tools/qshareddata.cpp b/src/corelib/tools/qshareddata.cpp index 345d7396b2b..ffc13f7d695 100644 --- a/src/corelib/tools/qshareddata.cpp +++ b/src/corelib/tools/qshareddata.cpp @@ -49,6 +49,10 @@ QT_BEGIN_NAMESPACE \since 4.0 \reentrant + \compares strong + \compareswith strong T* std::nullptr_t + \endcompareswith + QSharedDataPointer\ makes writing your own \l {implicitly shared} classes easy. QSharedDataPointer implements \l {thread-safe} reference counting, ensuring that adding QSharedDataPointers to your @@ -331,13 +335,13 @@ QT_BEGIN_NAMESPACE \e{d pointer}. This function does \e not call detach(). */ -/*! \fn template bool QSharedDataPointer::operator==(const T *ptr, const QSharedDataPointer& rhs) - Returns \c true if the \e{d pointer} of \a rhs is \a ptr. +/*! \fn template bool QSharedDataPointer::operator==(T* const &lhs, const QSharedDataPointer& rhs) + Returns \c true if the \e{d pointer} of \a rhs is \a lhs. This function does \e not call detach(). */ -/*! \fn template bool QSharedDataPointer::operator!=(const T *ptr, const QSharedDataPointer& rhs) - Returns \c true if the \e{d pointer} of \a rhs is \e not \a ptr. +/*! \fn template bool QSharedDataPointer::operator!=(T* const &lhs, const QSharedDataPointer& rhs) + Returns \c true if the \e{d pointer} of \a rhs is \e not \a lhs. \e{d pointer}. This function does \e not call detach(). */ diff --git a/src/corelib/tools/qshareddata.h b/src/corelib/tools/qshareddata.h index 8e96db93b5b..a9d41803391 100644 --- a/src/corelib/tools/qshareddata.h +++ b/src/corelib/tools/qshareddata.h @@ -6,6 +6,7 @@ #include #include +#include #include #include @@ -38,22 +39,22 @@ public: typedef T *pointer; void detach() { if (d && d->ref.loadRelaxed() != 1) detach_helper(); } - T &operator*() { detach(); return *d; } - const T &operator*() const { return *d; } - T *operator->() { detach(); return d; } - const T *operator->() const noexcept { return d; } - operator T *() { detach(); return d; } - operator const T *() const noexcept { return d; } - T *data() { detach(); return d; } - T *get() { detach(); return d; } - const T *data() const noexcept { return d; } - const T *get() const noexcept { return d; } - const T *constData() const noexcept { return d; } - T *take() noexcept { return std::exchange(d, nullptr); } + T &operator*() { detach(); return *(d.get()); } + const T &operator*() const { return *(d.get()); } + T *operator->() { detach(); return d.get(); } + const T *operator->() const noexcept { return d.get(); } + operator T *() { detach(); return d.get(); } + operator const T *() const noexcept { return d.get(); } + T *data() { detach(); return d.get(); } + T *get() { detach(); return d.get(); } + const T *data() const noexcept { return d.get(); } + const T *get() const noexcept { return d.get(); } + const T *constData() const noexcept { return d.get(); } + T *take() noexcept { return std::exchange(d, nullptr).get(); } Q_NODISCARD_CTOR QSharedDataPointer() noexcept : d(nullptr) { } - ~QSharedDataPointer() { if (d && !d->ref.deref()) delete d; } + ~QSharedDataPointer() { if (d && !d->ref.deref()) delete d.get(); } Q_NODISCARD_CTOR explicit QSharedDataPointer(T *data) noexcept : d(data) @@ -67,10 +68,10 @@ public: void reset(T *ptr = nullptr) noexcept { - if (ptr != d) { + if (ptr != d.get()) { 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; } @@ -78,7 +79,7 @@ public: QSharedDataPointer &operator=(const QSharedDataPointer &o) noexcept { - reset(o.d); + reset(o.d.get()); return *this; } inline QSharedDataPointer &operator=(T *o) noexcept @@ -96,33 +97,34 @@ public: void swap(QSharedDataPointer &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{}(A1, A2); } \ - friend bool operator<=(T1, T2) noexcept \ - { return !std::less{}(A2, A1); } \ - friend bool operator>(T1, T2) noexcept \ - { return std::less{}(A2, A1); } \ - friend bool operator>=(T1, T2) noexcept \ - { return !std::less{}(A1, A2); } \ - friend bool operator==(T1, T2) noexcept \ - { return A1 == A2; } \ - friend bool operator!=(T1, T2) noexcept \ - { return A1 != A2; } \ - - DECLARE_COMPARE_SET(const QSharedDataPointer &p1, p1.d, const QSharedDataPointer &p2, p2.d) - DECLARE_COMPARE_SET(const QSharedDataPointer &p1, p1.d, const T *ptr, ptr) - DECLARE_COMPARE_SET(const T *ptr, ptr, const QSharedDataPointer &p2, p2.d) - DECLARE_COMPARE_SET(const QSharedDataPointer &p1, p1.d, std::nullptr_t, nullptr) - DECLARE_COMPARE_SET(std::nullptr_t, nullptr, const QSharedDataPointer &p2, p2.d) - protected: T *clone(); private: + friend bool comparesEqual(const QSharedDataPointer &lhs, const QSharedDataPointer &rhs) noexcept + { return lhs.d == rhs.d; } + friend Qt::strong_ordering + compareThreeWay(const QSharedDataPointer &lhs, const QSharedDataPointer &rhs) noexcept + { return Qt::compareThreeWay(lhs.d, rhs.d); } + Q_DECLARE_STRONGLY_ORDERED(QSharedDataPointer) + + friend bool comparesEqual(const QSharedDataPointer &lhs, const T *rhs) noexcept + { return lhs.d == rhs; } + friend Qt::strong_ordering + compareThreeWay(const QSharedDataPointer &lhs, const T *rhs) noexcept + { return Qt::compareThreeWay(lhs.d, rhs); } + Q_DECLARE_STRONGLY_ORDERED(QSharedDataPointer, T*) + + friend bool comparesEqual(const QSharedDataPointer &lhs, std::nullptr_t) noexcept + { return lhs.d == nullptr; } + friend Qt::strong_ordering + compareThreeWay(const QSharedDataPointer &lhs, std::nullptr_t) noexcept + { return Qt::compareThreeWay(lhs.d, nullptr); } + Q_DECLARE_STRONGLY_ORDERED(QSharedDataPointer, std::nullptr_t) + void detach_helper(); - T *d; + Qt::totally_ordered_wrapper d; }; template @@ -198,6 +200,20 @@ 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{}(A1, A2); } \ + friend bool operator<=(T1, T2) noexcept \ + { return !std::less{}(A2, A1); } \ + friend bool operator>(T1, T2) noexcept \ + { return std::less{}(A2, A1); } \ + friend bool operator>=(T1, T2) noexcept \ + { return !std::less{}(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) @@ -227,9 +243,9 @@ Q_OUTOFLINE_TEMPLATE void QSharedDataPointer::detach_helper() { T *x = clone(); x->ref.ref(); - if (!d->ref.deref()) - delete d; - d = x; + if (!d.get()->ref.deref()) + delete d.get(); + d.reset(x); } template @@ -280,7 +296,7 @@ template Q_DECLARE_TYPEINFO_BODY(QExplicitlySharedDataPointer, Q_ template<> QSharedDataPointer::~QSharedDataPointer() \ { \ if (d && !d->ref.deref()) \ - delete d; \ + delete d.get(); \ } #define QT_DECLARE_QESDP_SPECIALIZATION_DTOR(Class) \ diff --git a/src/network/kernel/qnetworkproxy.cpp b/src/network/kernel/qnetworkproxy.cpp index 9b91b11d6b9..fbf4456742a 100644 --- a/src/network/kernel/qnetworkproxy.cpp +++ b/src/network/kernel/qnetworkproxy.cpp @@ -450,8 +450,8 @@ template<> void QSharedDataPointer::detach() : new QNetworkProxyPrivate); x->ref.ref(); if (d && !d->ref.deref()) - delete d; - d = x; + delete d.get(); + d.reset(x); } /*! @@ -937,8 +937,8 @@ template<> void QSharedDataPointer::detach() : new QNetworkProxyQueryPrivate); x->ref.ref(); if (d && !d->ref.deref()) - delete d; - d = x; + delete d.get(); + d.reset(x); } /*! diff --git a/tests/auto/corelib/tools/CMakeLists.txt b/tests/auto/corelib/tools/CMakeLists.txt index 5d4d4e99989..ed8c27a5f5d 100644 --- a/tests/auto/corelib/tools/CMakeLists.txt +++ b/tests/auto/corelib/tools/CMakeLists.txt @@ -44,6 +44,7 @@ add_subdirectory(qscopeguard) add_subdirectory(qtaggedpointer) add_subdirectory(qtyperevision) add_subdirectory(qset) +add_subdirectory(qshareddatapointer) add_subdirectory(qsharedpointer) add_subdirectory(qsize) add_subdirectory(qsizef) diff --git a/tests/auto/corelib/tools/qshareddatapointer/CMakeLists.txt b/tests/auto/corelib/tools/qshareddatapointer/CMakeLists.txt new file mode 100644 index 00000000000..d0de31e48e6 --- /dev/null +++ b/tests/auto/corelib/tools/qshareddatapointer/CMakeLists.txt @@ -0,0 +1,19 @@ +# Copyright (C) 2024 The Qt Company Ltd. +# SPDX-License-Identifier: BSD-3-Clause + +##################################################################### +## tst_qshareddatapointer Test: +##################################################################### + +if(NOT QT_BUILD_STANDALONE_TESTS AND NOT QT_BUILDING_QT) + cmake_minimum_required(VERSION 3.16) + project(tst_qshareddatapointer LANGUAGES CXX) + find_package(Qt6BuildInternals REQUIRED COMPONENTS STANDALONE_TEST) +endif() + +qt_internal_add_test(tst_qshareddatapointer + SOURCES + tst_qshareddatapointer.cpp + LIBRARIES + Qt::TestPrivate +) diff --git a/tests/auto/corelib/tools/qshareddatapointer/tst_qshareddatapointer.cpp b/tests/auto/corelib/tools/qshareddatapointer/tst_qshareddatapointer.cpp new file mode 100644 index 00000000000..d64013f1e98 --- /dev/null +++ b/tests/auto/corelib/tools/qshareddatapointer/tst_qshareddatapointer.cpp @@ -0,0 +1,63 @@ +// Copyright (C) 2024 The Qt Company Ltd. +// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only + +#include +#include + +#include + +class tst_QSharedDataPointer : public QObject +{ + Q_OBJECT + +private slots: + void compareCompiles(); + void compare(); +}; + +class MyClass : public QSharedData +{ +public: + MyClass(int v) : m_value(v) + { + ref.ref(); + }; + int m_value; +}; + +void tst_QSharedDataPointer::compareCompiles() +{ + QTestPrivate::testAllComparisonOperatorsCompile>(); + QTestPrivate::testAllComparisonOperatorsCompile, MyClass*>(); + QTestPrivate::testAllComparisonOperatorsCompile, std::nullptr_t>(); +} + +void tst_QSharedDataPointer::compare() +{ + const QSharedDataPointer ptr; + const QSharedDataPointer 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 QSharedDataPointer copy(ptr); + QT_TEST_ALL_COMPARISON_OPS(ptr, copy, Qt::strong_ordering::equal); + + const MyClass* pointer = nullptr; + QT_TEST_ALL_COMPARISON_OPS(pointer, ptr, Qt::strong_ordering::equal); + + const QSharedDataPointer pointer2(new MyClass(0), QAdoptSharedDataTag{}); + QT_TEST_ALL_COMPARISON_OPS(pointer, pointer2, Qt::strong_ordering::less); + + std::array myArray {MyClass(1), MyClass(0)}; + const QSharedDataPointer val0(&myArray[0]); + const QSharedDataPointer 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_QSharedDataPointer) + +#include "tst_qshareddatapointer.moc"