From f1c59a351ad34ece0e4823eef2482d9830d3deaa Mon Sep 17 00:00:00 2001 From: Volker Hilsheimer Date: Wed, 13 Dec 2023 19:16:14 +0100 Subject: [PATCH] JNI API review: use has-a-QJniObject relationship for QtJniTypes types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Don't subclass QJniObject. It's not necessary, and gets us into UB territory due to QJniObject not having a virtual destructor. Also, rename the helper class to JObject, as Object is a valid Java class that one might want to be able to declare explicitly. Instead, give the declared QtJniTypes types a QJniObject member that they forward the calls to. That requires some duplication of APIs, but at the same time makes it unnecessary to explicitly remove the old QJniObject APIs that we want to ultimately deprecate. We need to specialize a few more of the conversion routines to handle such types now, as QJniObject is no longer a base class. To be able to do that we need to add a base class that we can test for, and that has the APIs that don't depend on the template parameter. Since we now need to know about QtJniTypes::JObject(Base) in the conversion routines that are implemented in qjniobject.h, we have to move these base types into that header as well. This reduces the qjnitypes.h header to the macros for declaring types and a few helpers for native methods, which is perhaps how it should be anyway. Change-Id: If2052a79a108fdb62ca71c88f4fa04d9f5ea2c4b Reviewed-by: Mårten Nordheim (cherry picked from commit a5d14a9f5cfe41784960fe825609d91b9a18c17e) Reviewed-by: Qt Cherry-pick Bot --- src/corelib/kernel/qjniarray.h | 2 + src/corelib/kernel/qjniobject.h | 143 ++++++++++++++++++++++++- src/corelib/kernel/qjnitypes.h | 178 +------------------------------- 3 files changed, 148 insertions(+), 175 deletions(-) diff --git a/src/corelib/kernel/qjniarray.h b/src/corelib/kernel/qjniarray.h index 45418047252..743314f12b4 100644 --- a/src/corelib/kernel/qjniarray.h +++ b/src/corelib/kernel/qjniarray.h @@ -204,6 +204,8 @@ public: jobject element = env->GetObjectArrayElement(object(), i); if constexpr (std::is_base_of_v) return QJniObject::fromLocalRef(element); + else if constexpr (std::is_base_of_v) + return T::fromLocalRef(element); else return T{element}; } else { diff --git a/src/corelib/kernel/qjniobject.h b/src/corelib/kernel/qjniobject.h index 681d2bcec0f..ee111b49cd9 100644 --- a/src/corelib/kernel/qjniobject.h +++ b/src/corelib/kernel/qjniobject.h @@ -662,6 +662,144 @@ inline bool operator!=(const QJniObject &obj1, const QJniObject &obj2) return !obj1.isSameObject(obj2); } +namespace QtJniTypes { +struct JObjectBase +{ + operator QJniObject() const { return m_object; } + + bool isValid() const { return m_object.isValid(); } + jclass objectClass() const { return m_object.objectClass(); } + QString toString() const { return m_object.toString(); } + + template + T object() const { + return m_object.object(); + } + +protected: + JObjectBase() = default; + ~JObjectBase() = default; + + Q_IMPLICIT JObjectBase(jobject object) : m_object(object) {} + Q_IMPLICIT JObjectBase(const QJniObject &object) : m_object(object) {} + Q_IMPLICIT JObjectBase(QJniObject &&object) noexcept : m_object(std::move(object)) {} + + QJniObject m_object; +}; + +template +class JObject : public JObjectBase +{ +public: + using Class = Type; + + JObject() + : JObjectBase{QJniObject(QtJniTypes::Traits::className())} + {} + Q_IMPLICIT JObject(jobject object) : JObjectBase(object) {} + Q_IMPLICIT JObject(const QJniObject &object) : JObjectBase(object) {} + Q_IMPLICIT JObject(QJniObject &&object) noexcept : JObjectBase(std::move(object)) {} + + // base class destructor is protected, so need to provide all SMFs + JObject(const JObject &other) = default; + JObject(JObject &&other) noexcept = default; + JObject &operator=(const JObject &other) = default; + JObject &operator=(JObject &&other) noexcept = default; + + ~JObject() = default; + + template, bool> = true + , IfValidSignatureTypes = true + > + explicit JObject(Arg && arg, Args &&...args) + : JObjectBase{QJniObject(QtJniTypes::Traits::className(), + std::forward(arg), std::forward(args)...)} + {} + + // named constructors avoid ambiguities + static Type fromJObject(jobject object) { return Type(object); } + template + static Type construct(Args &&...args) { return Type(std::forward(args)...); } + static Type fromLocalRef(jobject lref) { return Type(QJniObject::fromLocalRef(lref)); } + + static bool registerNativeMethods(std::initializer_list methods) + { + QJniEnvironment env; + return env.registerNativeMethods(methods); + } + + // public API forwarding to QJniObject, with the implicit Class template parameter + template = true +#endif + > + static auto callStaticMethod(const char *name, Args &&...args) + { + return QJniObject::callStaticMethod(name, + std::forward(args)...); + } + template = true +#endif + > + static auto getStaticField(const char *field) + { + return QJniObject::getStaticField(field); + } + template = true +#endif + > + static void setStaticField(const char *field, T &&value) + { + QJniObject::setStaticField(field, std::forward(value)); + } + + // keep only these overloads, the rest is made private + template = true +#endif + > + auto callMethod(const char *method, Args &&...args) const + { + return m_object.callMethod(method, std::forward(args)...); + } + template = true +#endif + > + auto getField(const char *fieldName) const + { + return m_object.getField(fieldName); + } + + template = true +#endif + > + void setField(const char *fieldName, T &&value) + { + m_object.setField(fieldName, std::forward(value)); + } + + QByteArray className() const { + return QtJniTypes::Traits::className().data(); + } + +private: + friend bool comparesEqual(const JObject &lhs, const JObject &rhs) noexcept + { return lhs.m_object == rhs.m_object; } + Q_DECLARE_EQUALITY_COMPARABLE_LITERAL_TYPE(JObject); +}; +} + // This cannot be included earlier as QJniArray is a QJniObject subclass, but it // must be included so that we can implement QJniObject::LocalFrame conversion. QT_END_NAMESPACE @@ -681,7 +819,8 @@ auto QJniObject::LocalFrame::convertToJni(T &&value) using QJniArrayType = decltype(QJniArrayBase::fromContainer(std::forward(value))); using ArrayType = decltype(std::declval().arrayObject()); return newLocalRef(QJniArrayBase::fromContainer(std::forward(value)).template object()); - } else if constexpr (std::is_base_of_v) { + } else if constexpr (std::is_base_of_v + || std::is_base_of_v) { return value.object(); } else { return std::forward(value); @@ -710,6 +849,8 @@ auto QJniObject::LocalFrame::convertFromJni(QJniObject &&object) } else if constexpr (std::is_base_of_v && !std::is_same_v) { return T{std::move(object)}; + } else if constexpr (std::is_base_of_v) { + return T{std::move(object)}; } else { return std::move(object); } diff --git a/src/corelib/kernel/qjnitypes.h b/src/corelib/kernel/qjnitypes.h index b129afb50b1..56af657fa30 100644 --- a/src/corelib/kernel/qjnitypes.h +++ b/src/corelib/kernel/qjnitypes.h @@ -11,182 +11,11 @@ QT_BEGIN_NAMESPACE -namespace QtJniTypes -{ -// A generic base class for specialized QJniObject types, to be used by -// subclasses via CRTP. It's implicitly convertible to and from jobject, which -// allows the QJniObject implementation to implicitly pass instances of this -// type through the variadic argument JNI APIs. -template -struct Object : QJniObject -{ - using Class = Type; - Q_IMPLICIT Object(jobject object) : QJniObject(object) {} - Q_IMPLICIT Object(const QJniObject &object) : QJniObject(object) {} - Q_IMPLICIT Object(QJniObject &&object) : QJniObject(std::move(object)) {} - - // Compiler-generated copy/move semantics based on QJniObject's shared d-pointer are fine! - Object(const Object &other) = default; - Object(Object &&other) = default; - Object &operator=(const Object &other) = default; - Object &operator=(Object &&other) = default; - - // avoid ambiguities with deleted const char * constructor - Q_IMPLICIT Object(std::nullptr_t) : QJniObject() {} - - Object() - : QJniObject(QtJniTypes::Traits::className()) - {} - - template, bool> = true - , IfValidSignatureTypes = true - > - explicit Object(Arg && arg, Args &&...args) - : QJniObject(QtJniTypes::Traits::className(), std::forward(arg), - std::forward(args)...) - {} - - // named constructors avoid ambiguities - static Object fromJObject(jobject object) { return Object(object); } - template - static Object construct(Args &&...args) { return Object{std::forward(args)...}; } - - static bool registerNativeMethods(std::initializer_list methods) - { - QJniEnvironment env; - return env.registerNativeMethods(methods); - } - - // public API forwarding to QJniObject, with the implicit Class template parameter - template = true -#endif - > - static auto callStaticMethod(const char *name, Args &&...args) - { - return QJniObject::callStaticMethod(name, - std::forward(args)...); - } - template = true -#endif - > - static auto getStaticField(const char *field) - { - return QJniObject::getStaticField(field); - } - template = true -#endif - > - static void setStaticField(const char *field, T &&value) - { - QJniObject::setStaticField(field, std::forward(value)); - } - - // keep only these overloads, the rest is made private - template = true -#endif - > - auto callMethod(const char *method, Args &&...args) const - { - return QJniObject::callMethod(method, std::forward(args)...); - } - template = true -#endif - > - auto getField(const char *fieldName) const - { - return QJniObject::getField(fieldName); - } - - template = true -#endif - > - void setField(const char *fieldName, T &&value) - { - QJniObject::setField(fieldName, std::forward(value)); - } - -private: - // The following declutters the API of these types compared to the QJniObject API. - // 1) 'Object' methods; the functions we have have return type auto and will return - // the type specified by the template parameter. - using QJniObject::callObjectMethod; - using QJniObject::callStaticObjectMethod; - using QJniObject::getObjectField; - using QJniObject::getStaticObjectField; - - // 2) Constructors that take a class name, signature string, or class template argument - explicit Object(const char *className) = delete; - explicit Object(const char *className, const char *signature, ...) = delete; - template - explicit Object(const char *className, Args &&...args) = delete; - explicit Object(jclass clazz, const char *signature, ...) = delete; - template - static QJniObject construct(Args &&...args) = delete; - - // 3) Overloads that take a class name/jclass, methodID, signature string, or an - // explicit class template argument - template - auto callMethod(const char *methodName, const char *signature, Args &&...args) const = delete; - template - - static auto callStaticMethod(const char *className, const char *methodName, - const char *signature, Args &&...args) = delete; - template - static auto callStaticMethod(jclass clazz, const char *methodName, - const char *signature, Args &&...args) = delete; - template - static auto callStaticMethod(jclass clazz, jmethodID methodId, Args &&...args) = delete; - template - static auto callStaticMethod(const char *className, const char *methodName, - Args &&...args) = delete; - template - static auto callStaticMethod(jclass clazz, const char *methodName, Args &&...args) = delete; - template - static auto callStaticMethod(const char *methodName, Args &&...args) = delete; - - template - static auto getStaticField(const char *className, const char *fieldName) = delete; - template - static auto getStaticField(jclass clazz, const char *fieldName) = delete; - template - static auto getStaticField(const char *fieldName) = delete; - - template - void setField(const char *fieldName, const char *signature, T value) = delete; - template - static void setStaticField(const char *className, const char *fieldName, T value) = delete; - template - static void setStaticField(const char *className, const char *fieldName, - const char *signature, T value) = delete; - template - static void setStaticField(jclass clazz, const char *fieldName, - const char *signature, T value) = delete; - template - static void setStaticField(jclass clazz, const char *fieldName, T value) = delete; - template - static void setStaticField(const char *fieldName, T value) = delete; -}; - -} // namespace QtJniTypes - #define Q_DECLARE_JNI_TYPE_HELPER(Type) \ namespace QtJniTypes { \ -struct Type : Object \ +struct Type : JObject \ { \ - using Object::Object; \ + using JObject::JObject; \ }; \ } \ @@ -246,7 +75,8 @@ template <> struct PromotedType { using Type = double; }; template struct JNITypeForArgImpl { - using Type = std::conditional_t, + using Type = std::conditional_t, + std::is_base_of>, jobject, typename PromotedType::Type>; static Arg fromVarArg(Type &&t) {