From c0914c11a88a31d4b5a7d533ab4a3eb4afb5725d Mon Sep 17 00:00:00 2001 From: Rym Bouabid Date: Wed, 3 Apr 2024 19:11:08 +0200 Subject: [PATCH] QExplicitlySharedDataPointerV2: Use new comparison helper macros Provide the new comparesEqual() helper function as an implementation of the (in)equality operators. The smart pointers are totally ordered, so generalize QExplicitlySharedDataPointerV2 to the full comparison set by adding compareThreeWay() helper function. Use Q_DECLARE_STRONGLY_ORDERED_LITERAL_TYPE to provide all relational operators. Add comparisons between QExplicitlySharedDataPointerV2 and std::nullptr_t. Use QT_TEST_ALL_COMPARISON_OPS macros in unit-tests. Use the new Qt::totally_ordered_wrapper to wrap the "d" pointer to avoid UB when performing comparisons of QExplicitlySharedDataPointerV2. Task-number: QTBUG-120306 Change-Id: I177fdc1ff2363a5e84e65506468093b87c5c9c03 Reviewed-by: Ivan Solovev --- src/corelib/tools/qshareddata_impl.h | 53 +++++++++++-------- .../CMakeLists.txt | 2 + .../tst_qexplicitlyshareddatapointerv2.cpp | 52 ++++++++++-------- 3 files changed, 63 insertions(+), 44 deletions(-) diff --git a/src/corelib/tools/qshareddata_impl.h b/src/corelib/tools/qshareddata_impl.h index e0b4695e362..72df57374c5 100644 --- a/src/corelib/tools/qshareddata_impl.h +++ b/src/corelib/tools/qshareddata_impl.h @@ -9,6 +9,7 @@ #ifndef QSHAREDDATA_IMPL_H #define QSHAREDDATA_IMPL_H +#include #include #include @@ -19,7 +20,7 @@ namespace QtPrivate { template class QExplicitlySharedDataPointerV2 { - T *d; + Qt::totally_ordered_wrapper d; public: constexpr QExplicitlySharedDataPointerV2() noexcept : d(nullptr) {} @@ -65,14 +66,14 @@ public: ~QExplicitlySharedDataPointerV2() { if (d && !d->ref.deref()) - delete d; + delete d.get(); } void detach() { if (!d) { // should this codepath be here on in all user's detach()? - d = new T; + d.reset(new T); d->ref.ref(); } else if (d->ref.loadRelaxed() != 1) { // TODO: qAtomicDetach here...? @@ -84,15 +85,15 @@ public: void reset(T *t = nullptr) noexcept { if (d && !d->ref.deref()) - delete d; - d = t; + delete d.get(); + d.reset(t); if (d) d->ref.ref(); } constexpr T *take() noexcept { - return std::exchange(d, nullptr); + return std::exchange(d, nullptr).get(); } bool isShared() const noexcept @@ -106,27 +107,33 @@ public: } // important change from QExplicitlySharedDataPointer: deep const - constexpr T &operator*() { return *d; } - constexpr T *operator->() { return d; } - constexpr const T &operator*() const { return *d; } - constexpr const T *operator->() const { return d; } + constexpr T &operator*() { return *(d.get()); } + constexpr T *operator->() { return d.get(); } + constexpr const T &operator*() const { return *(d.get()); } + constexpr const T *operator->() const { return d.get(); } - constexpr T *data() noexcept { return d; } - constexpr const T *data() const noexcept { return d; } + constexpr T *data() noexcept { return d.get(); } + constexpr const T *data() const noexcept { return d.get(); } - constexpr explicit operator bool() const noexcept { return d; } + constexpr explicit operator bool() const noexcept { return d.get(); } - constexpr friend bool operator==(const QExplicitlySharedDataPointerV2 &lhs, - const QExplicitlySharedDataPointerV2 &rhs) noexcept - { - return lhs.d == rhs.d; - } +private: + constexpr friend bool comparesEqual(const QExplicitlySharedDataPointerV2 &lhs, + const QExplicitlySharedDataPointerV2 &rhs) noexcept + { return lhs.d == rhs.d; } + constexpr friend Qt::strong_ordering + compareThreeWay(const QExplicitlySharedDataPointerV2 &lhs, + const QExplicitlySharedDataPointerV2 &rhs) noexcept + { return Qt::compareThreeWay(lhs.d, rhs.d); } + Q_DECLARE_STRONGLY_ORDERED_LITERAL_TYPE(QExplicitlySharedDataPointerV2) - constexpr friend bool operator!=(const QExplicitlySharedDataPointerV2 &lhs, - const QExplicitlySharedDataPointerV2 &rhs) noexcept - { - return lhs.d != rhs.d; - } + constexpr friend bool + comparesEqual(const QExplicitlySharedDataPointerV2 &lhs, std::nullptr_t) noexcept + { return lhs.d == nullptr; } + constexpr friend Qt::strong_ordering + compareThreeWay(const QExplicitlySharedDataPointerV2 &lhs, std::nullptr_t) noexcept + { return Qt::compareThreeWay(lhs.d, nullptr); } + Q_DECLARE_STRONGLY_ORDERED_LITERAL_TYPE(QExplicitlySharedDataPointerV2, std::nullptr_t) }; template diff --git a/tests/auto/corelib/tools/qexplicitlyshareddatapointerv2/CMakeLists.txt b/tests/auto/corelib/tools/qexplicitlyshareddatapointerv2/CMakeLists.txt index 553450907e4..e40ab03a31a 100644 --- a/tests/auto/corelib/tools/qexplicitlyshareddatapointerv2/CMakeLists.txt +++ b/tests/auto/corelib/tools/qexplicitlyshareddatapointerv2/CMakeLists.txt @@ -14,4 +14,6 @@ endif() qt_internal_add_test(tst_qexplicitlyshareddatapointerv2 SOURCES tst_qexplicitlyshareddatapointerv2.cpp + LIBRARIES + Qt::TestPrivate ) diff --git a/tests/auto/corelib/tools/qexplicitlyshareddatapointerv2/tst_qexplicitlyshareddatapointerv2.cpp b/tests/auto/corelib/tools/qexplicitlyshareddatapointerv2/tst_qexplicitlyshareddatapointerv2.cpp index d8493306250..c653af3def8 100644 --- a/tests/auto/corelib/tools/qexplicitlyshareddatapointerv2/tst_qexplicitlyshareddatapointerv2.cpp +++ b/tests/auto/corelib/tools/qexplicitlyshareddatapointerv2/tst_qexplicitlyshareddatapointerv2.cpp @@ -2,6 +2,7 @@ // SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only #include +#include #include @@ -16,6 +17,7 @@ private slots: void moveConstructor() const; void copyAssignment() const; void moveAssignment() const; + void compareCompiles() const; void compare() const; void mutability() const; void data() const; @@ -46,8 +48,8 @@ void tst_QExplicitlySharedDataPointerv2::moveConstructor() const { QESDP_V2 pointer(new MyClass()); const QESDP_V2 moved(std::move(pointer)); - QCOMPARE_NE(moved.data(), static_cast(0)); - QCOMPARE_EQ(pointer.data(), static_cast(0)); + QCOMPARE_NE(moved, nullptr); + QCOMPARE_EQ(pointer, nullptr); } void tst_QExplicitlySharedDataPointerv2::copyAssignment() const @@ -61,29 +63,35 @@ void tst_QExplicitlySharedDataPointerv2::moveAssignment() const { QESDP_V2 pointer(new MyClass()); const QESDP_V2 moved = std::move(pointer); - QCOMPARE_NE(moved.data(), static_cast(0)); - QCOMPARE_EQ(pointer.data(), static_cast(0)); + QCOMPARE_NE(moved, nullptr); + QCOMPARE_EQ(pointer, nullptr); +} + +void tst_QExplicitlySharedDataPointerv2::compareCompiles() const +{ + QTestPrivate::testAllComparisonOperatorsCompile>(); + QTestPrivate::testAllComparisonOperatorsCompile, std::nullptr_t>(); } void tst_QExplicitlySharedDataPointerv2::compare() const { const QESDP_V2 ptr; const QESDP_V2 ptr2; - QCOMPARE_EQ(ptr.data(), static_cast(0)); - QCOMPARE_EQ(ptr2.data(), static_cast(0)); - QCOMPARE_EQ(ptr, 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 QESDP_V2 copy(ptr); - QCOMPARE_EQ(ptr, copy); + QT_TEST_ALL_COMPARISON_OPS(ptr, copy, Qt::strong_ordering::equal); const QESDP_V2 new_ptr(new MyClass()); - QCOMPARE_NE(new_ptr.data(), static_cast(0)); - QCOMPARE_NE(ptr, new_ptr); + QT_TEST_ALL_COMPARISON_OPS(new_ptr, nullptr, Qt::strong_ordering::greater); + QT_TEST_ALL_COMPARISON_OPS(new_ptr, ptr, Qt::strong_ordering::greater); std::array myArray {MyClass(2), MyClass(1), MyClass(0)}; const QESDP_V2 val0(&myArray[0]); const QESDP_V2 val1(&myArray[1]); - QCOMPARE_NE(val1, val0); + QT_TEST_ALL_COMPARISON_OPS(val0, val1, Qt::strong_ordering::less); } void tst_QExplicitlySharedDataPointerv2::mutability() const @@ -121,12 +129,14 @@ void tst_QExplicitlySharedDataPointerv2::data() const { QESDP_V2 pointer; QCOMPARE_EQ(pointer.data(), static_cast(0)); + QT_TEST_ALL_COMPARISON_OPS(pointer, nullptr, Qt::strong_ordering::equal); } { const QESDP_V2 pointer(new MyClass()); /* Check that this cast is possible. */ Q_UNUSED(static_cast(pointer.data())); + QT_TEST_ALL_COMPARISON_OPS(pointer, nullptr, Qt::strong_ordering::greater); } { @@ -154,31 +164,31 @@ void tst_QExplicitlySharedDataPointerv2::reset() const /* Reset a default constructed shared data object: reference count is equal to 0. */ { QESDP_V2 pointer; - QCOMPARE_EQ(pointer.data(), static_cast(0)); + QCOMPARE_EQ(pointer, nullptr); pointer.reset(); - QCOMPARE_EQ(pointer.data(), static_cast(0)); + QCOMPARE_EQ(pointer, nullptr); } /* Reset a shared data object where the reference count is equal to 1. */ { QESDP_V2 pointer(new MyClass()); - QCOMPARE_NE(pointer.data(), static_cast(0)); + QCOMPARE_NE(pointer, nullptr); pointer.reset(); - QCOMPARE_EQ(pointer.data(), static_cast(0)); + QCOMPARE_EQ(pointer, nullptr); } /* Reset a shared data object where the reference count is greater than 1. */ { QESDP_V2 pointer(new MyClass()); - QCOMPARE_NE(pointer.data(), static_cast(0)); + QCOMPARE_NE(pointer, nullptr); QESDP_V2 pointer2(pointer); - QCOMPARE_NE(pointer2.data(), static_cast(0)); + QCOMPARE_NE(pointer2, nullptr); pointer.reset(); - QCOMPARE_EQ(pointer.data(), static_cast(0)); - QCOMPARE_NE(pointer2.data(), static_cast(0)); + QCOMPARE_EQ(pointer, nullptr); + QCOMPARE_NE(pointer2, nullptr); } } @@ -204,9 +214,9 @@ void tst_QExplicitlySharedDataPointerv2::swap() const void tst_QExplicitlySharedDataPointerv2::take() const { QESDP_V2 pointer(new MyClass()); - QCOMPARE_NE(pointer.data(), static_cast(0)); + QCOMPARE_NE(pointer, nullptr); delete pointer.take(); - QCOMPARE_EQ(pointer.data(), static_cast(0)); + QCOMPARE_EQ(pointer, nullptr); } QTEST_MAIN(tst_QExplicitlySharedDataPointerv2)