From c4bd8a54924ce51d381d19a5c0f5c08a38f26e2b Mon Sep 17 00:00:00 2001 From: Volker Hilsheimer Date: Wed, 10 Jul 2024 17:25:43 +0200 Subject: [PATCH] JNI: clean up move semantics for QJniArray MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Explicitly define (compiler-implemented) copy and move SMF's for QJniArrayBase; we need to, as the destructor is declared, so without them the compiler will implicitly convert an array to a QJniObject and call that constructor. Constrain the constructors and assignment operator from a QJniArray of another type so that no narrowing conversion is allowed. Due to the implicit conversion to QJniObject, we have to explicitly delete the overload for narrowing conversions. Use the detector we have in qobjectdefs_impl.h for that. Make the detection helpers private, and add test coverage. Change-Id: I1b2bb4435d52223567d20bb55ceb0d516e3b0b15 Reviewed-by: Mårten Nordheim (cherry picked from commit ebbf7b0fdf866190cd20e62d6b13c7c6808101da) Reviewed-by: Qt Cherry-pick Bot --- src/corelib/kernel/qjniarray.h | 65 +++++++++++++++---- src/corelib/kernel/qjniarray.qdoc | 39 ++++++----- .../kernel/qjniarray/tst_qjniarray.cpp | 58 +++++++++++++++++ 3 files changed, 131 insertions(+), 31 deletions(-) diff --git a/src/corelib/kernel/qjniarray.h b/src/corelib/kernel/qjniarray.h index 3f4968361dc..2cb5a98128a 100644 --- a/src/corelib/kernel/qjniarray.h +++ b/src/corelib/kernel/qjniarray.h @@ -159,6 +159,13 @@ class QJniArrayBase > > : std::true_type {}; +protected: + // these are used in QJniArray + template + using if_convertible = std::enable_if_t::value, bool>; + template + using unless_convertible = std::enable_if_t::value, bool>; + public: using size_type = jsize; using difference_type = size_type; @@ -196,7 +203,7 @@ public: std::is_same, std::is_base_of >) { - return QJniArray(makeObjectArray(std::forward(container))); + return QJniArray(makeObjectArray(std::forward(container)).arrayObject()); } else if constexpr (QtJniTypes::sameTypeForJni) { return makeArray(std::forward(container), &JNIEnv::NewFloatArray, &JNIEnv::SetFloatArrayRegion); @@ -233,6 +240,11 @@ protected: QJniArrayBase() = default; ~QJniArrayBase() = default; + explicit QJniArrayBase(const QJniArrayBase &other) = default; + explicit QJniArrayBase(QJniArrayBase &&other) noexcept = default; + QJniArrayBase &operator=(const QJniArrayBase &other) = default; + QJniArrayBase &operator=(QJniArrayBase &&other) noexcept = default; + explicit QJniArrayBase(jarray array) : m_object(static_cast(array)) { @@ -243,6 +255,16 @@ protected: explicit QJniArrayBase(QJniObject &&object) noexcept : m_object(std::move(object)) {} + QJniArrayBase &operator=(const QJniObject &object) + { + m_object = object; + return *this; + } + QJniArrayBase &operator=(QJniObject &&object) noexcept + { + m_object = std::move(object); + return *this; + } JNIEnv *jniEnv() const noexcept { return QJniEnvironment::getJniEnv(); } @@ -286,11 +308,34 @@ public: explicit QJniArray(const QJniObject &object) : QJniArrayBase(object) {} explicit QJniArray(QJniObject &&object) noexcept : QJniArrayBase(std::move(object)) {} - // base class destructor is protected, so need to provide all SMFs - QJniArray(const QJniArray &other) = default; - QJniArray(QJniArray &&other) noexcept = default; - QJniArray &operator=(const QJniArray &other) = default; - QJniArray &operator=(QJniArray &&other) noexcept = default; + template = true> + QJniArray(const QJniArray &other) + : QJniArrayBase(other) + { + } + template = true> + QJniArray(QJniArray &&other) noexcept + : QJniArrayBase(std::move(other)) + { + } + template = true> + QJniArray &operator=(const QJniArray &other) + { + QJniArrayBase::operator=(QJniObject(other)); + return *this; + } + template = true> + QJniArray &operator=(QJniArray &&other) noexcept + { + QJniArray moved(std::move(other)); + swap(moved); + return *this; + } + // explicitly delete to disable detour via operator QJniObject() + template = true> + QJniArray(const QJniArray &other) = delete; + template = true> + QJniArray(QJniArray &&other) noexcept = delete; template = true> explicit QJniArray(Container &&container) @@ -303,14 +348,6 @@ public: { } - template - using if_convertible = std::enable_if_t, bool>; - - template = true> - QJniArray(QJniArray &&other) - : QJniArrayBase(std::forward>(other)) - { - } ~QJniArray() = default; auto arrayObject() const diff --git a/src/corelib/kernel/qjniarray.qdoc b/src/corelib/kernel/qjniarray.qdoc index ba2593fcc8f..4bc38d6c22a 100644 --- a/src/corelib/kernel/qjniarray.qdoc +++ b/src/corelib/kernel/qjniarray.qdoc @@ -234,17 +234,10 @@ */ /*! - \fn template QJniArray::QJniArray(const QJniArray &other) + \fn template template > QJniArray::QJniArray(const QJniArray &other) - Constructs a QJniArray that is a copy of \a other. Both QJniArray objects - will reference the same Java array object. -*/ - -/*! - \fn template template > QJniArray::QJniArray(QJniArray &&other) - - Constructs a QJniArray that is a copy of \a other, with both objects referencing - the same Java array object. + Constructs a QJniArray by copying \a other. Both QJniArray objects will + reference the same Java array object. This constructor only participates in overload resolution if the element type \c{Other} of \a other is convertible to the element type \c{T} of the @@ -252,24 +245,36 @@ */ /*! - \fn template QJniArray::QJniArray(QJniArray &&other) + \fn template template > QJniArray::QJniArray(QJniArray &&other) Constructs a QJniArray by moving from \a other. The \a other array becomes \l{QJniArrayBase::isValid}{invalid}. + + This constructor only participates in overload resolution if the element + type \c{Other} of \a other is convertible to the element type \c{T} of the + QJniArray being constructed. However, no actual conversion takes place. */ /*! - \fn template QJniArray::operator=(const QJniArray &other) + \fn template template > QJniArray &QJniArray::operator=(const QJniArray &other) - Assigns \a other to this QJniArray. Both QJniArray objects will reference - the same Java array object. + Assigns \a other to this QJniArray, and returns a reference to this. Both + QJniArray objects will reference the same Java array object. + + This operator only participates in overload resolution if the element + type \c{Other} of \a other is convertible to the element type \c{T} of this + QJniArray. However, no actual conversion takes place. */ /*! - \fn template QJniArray::operator=(QJniArray &&other) + \fn template template > QJniArray &QJniArray::operator=(QJniArray &&other) - Moves \a other into this QJniArray. The \a other array becomes - \l{QJniArrayBase::isValid}{invalid}. + Moves \a other into this QJniArray, and returns a reference to this. The + \a other array becomes \l{QJniArrayBase::isValid}{invalid}. + + This operator only participates in overload resolution if the element + type \c{Other} of \a other is convertible to the element type \c{T} of this + QJniArray. However, no actual conversion takes place. */ /*! diff --git a/tests/auto/corelib/kernel/qjniarray/tst_qjniarray.cpp b/tests/auto/corelib/kernel/qjniarray/tst_qjniarray.cpp index 6598679e159..78d147a1660 100644 --- a/tests/auto/corelib/kernel/qjniarray/tst_qjniarray.cpp +++ b/tests/auto/corelib/kernel/qjniarray/tst_qjniarray.cpp @@ -17,6 +17,7 @@ public: private slots: void construct(); + void copyAndMove(); void invalidArraysAreEmpty(); void size(); void operators(); @@ -183,6 +184,63 @@ void tst_QJniArray::construct() } } +// Verify that we can convert QJniArrays into each other as long as element types +// are convertible without narrowing. +template +using CanConstructDetector = decltype(QJniArray(std::declval>())); +template +using CanAssignDetector = decltype(std::declval>().operator=(std::declval>())); + +template +static constexpr bool canConstruct = qxp::is_detected_v; +template +static constexpr bool canAssign = qxp::is_detected_v; + +static_assert(canConstruct && canAssign); +static_assert(!canConstruct && !canAssign); +static_assert(canConstruct && canAssign); +static_assert(!canConstruct && !canAssign); + +// exercise the QJniArray(QJniArray &&other) constructor +void tst_QJniArray::copyAndMove() +{ + QJniArray tempShortArray({1, 2, 3}); + + // copy - both arrays remain valid and reference the same object + { + QJniArray shortArrayCopy(tempShortArray); + QVERIFY(tempShortArray.isValid()); + QVERIFY(shortArrayCopy.isValid()); + QCOMPARE(tempShortArray, shortArrayCopy); + } + + // moving QJniArray to QJniArray leaves the moved-from object invalid + QJniArray shortArray(std::move(tempShortArray)); + QVERIFY(!tempShortArray.isValid()); + QVERIFY(shortArray.isValid()); + + tempShortArray = shortArray; + + // copying QJniArray to QJniArray works + QJniArray intArray(shortArray); + QVERIFY(shortArray.isValid()); + QVERIFY(intArray.isValid()); + QCOMPARE(intArray, shortArray); + + // moving QJniArray to QJniArray leaves the moved-from array invalid + QJniArray longArray(std::move(shortArray)); + QVERIFY(!shortArray.isValid()); + QVERIFY(longArray.isValid()); + QCOMPARE(longArray, intArray); + QCOMPARE_NE(longArray, shortArray); // we can compare a moved-from object + + longArray = intArray; + + // not possible due to narrowing conversion, covered by static_asserts above + // QJniArray shortArray2(longArray); + // intArray = longArray; +} + void tst_QJniArray::invalidArraysAreEmpty() { QJniArray invalid;