JNI: refactor conversion logic into Traits struct

This disentangles and simplifies the code a lot, making it each
type's own responsibility to implement the conversion to and
from JNI. In order to be able to specialize the Traits for any
type with a certain predicate being true (e.g. all arrays, or
containers usable via QJniArray) we have to introduce a second,
defaulted template parameter through which we can SFINAE-in
relevant specializations.

LocalFrame's convertTo/FromJni now become thin wrappers around
the implementation in the Traits specialization, just taking
care of some special cases for local reference frame management.

Change-Id: Ic0e8af89f9e3d9a606b67d5238c1c793114ac38f
Reviewed-by: Assam Boudjelthia <assam.boudjelthia@qt.io>

src/corelib/kernel/qjniarray.h

@@ -325,7 +325,6 @@ private:
 
 template <typename T> // need to specialize traits for it, so can't be nested
 struct QJniArrayMutableValueRef {
-    using refwrapper = T;
     T value;
     QJniArrayMutableIterator<T> back = {-1, nullptr};
 
@@ -466,11 +465,17 @@ public:
     // forward-iterable container, so explicitly remove that from the overload
     // set so that the copy constructors get used instead.
     // Used also in the deduction guide, so must be public
+    template <typename C>
+    using IsSequentialOrContiguous = std::bool_constant<
+                                        IsSequentialContainerHelper<C>::isForwardIterable
+                                || (isContiguousContainer<C> && ElementTypeHelper<C>::isPrimitive)
+                                >;
     template <typename CRef, typename C = q20::remove_cvref_t<CRef>>
-    static constexpr bool isCompatibleSourceContainer =
+    static constexpr bool isCompatibleSourceContainer = std::conjunction_v<
-        (IsSequentialContainerHelper<C>::isForwardIterable
+        std::negation<std::is_same<QString, C>>,
-         || (isContiguousContainer<C> && ElementTypeHelper<C>::isPrimitive))
+        IsSequentialOrContiguous<C>,
-        && !std::is_base_of_v<QJniArrayBase, C>;
+        std::negation<std::is_base_of<QJniArrayBase, C>>
+    >;
 
     template <typename C>
     using if_compatible_source_container = std::enable_if_t<isCompatibleSourceContainer<C>, bool>;
@@ -963,29 +968,70 @@ auto QJniArrayBase::makeObjectArray(List &&list)
 
 namespace QtJniTypes
 {
-template <typename T> struct IsJniArray: std::false_type {};
+template <typename T> struct Traits<QJniArray<T>>
-template <typename T> struct IsJniArray<QJniArray<T>> : std::true_type {};
+{
-template <typename T> struct Traits<QJniArray<T>> {
     template <IfValidFieldType<T> = true>
     static constexpr auto signature()
     {
         return CTString("[") + Traits<T>::signature();
     }
+    static auto convertToJni(JNIEnv *, const QJniArray<T> &value)
+    {
+        return value.arrayObject();
+    }
+    static auto convertFromJni(QJniObject &&object)
+    {
+        return QJniArray<T>(std::move(object));
+    }
 };
 
 template <typename T> struct Traits<QJniArrayMutableValueRef<T>> : public Traits<T> {};
 
-template <typename T> struct Traits<QList<T>> {
+template<typename T> struct Traits<T, std::enable_if_t<QJniArrayBase::isCompatibleSourceContainer<T>>>
-    template <IfValidFieldType<T> = true>
+{
+    // QByteArray::value_type is char, which maps to 'C'; we need 'B', i.e. jbyte
+    using ElementType = std::conditional_t<std::is_same_v<T, QByteArray>,
+                                           jbyte, typename T::value_type>;
+
+    template <typename U = ElementType, IfValidFieldType<U> = true>
     static constexpr auto signature()
     {
-        return CTString("[") + Traits<T>::signature();
+        return CTString("[") + Traits<ElementType>::signature();
+    }
+
+    static auto convertToJni(JNIEnv *env, const T &value)
+    {
+        using QJniArrayType = decltype(QJniArrayBase::fromContainer(value));
+        using ArrayType = decltype(std::declval<QJniArrayType>().arrayObject());
+        return static_cast<ArrayType>(env->NewLocalRef(QJniArray(value).arrayObject()));
+    }
+
+    static auto convertFromJni(QJniObject &&object)
+    {
+        // if we were to create a QJniArray from Type...
+        using QJniArrayType = decltype(QJniArrayBase::fromContainer(std::declval<T>()));
+        // then that QJniArray would have elements of type
+        using ArrayType = typename QJniArrayType::Type;
+        // construct a QJniArray from a jobject pointer of that type
+        return QJniArray<ArrayType>(object.template object<jarray>()).toContainer();
     }
 };
-template <> struct Traits<QByteArray> {
+
+template<typename T> struct Traits<T, std::enable_if_t<std::is_array_v<T>>>
+{
+    using ElementType = std::remove_extent_t<T>;
+
+    template <typename U = ElementType, IfValidFieldType<U> = true>
     static constexpr auto signature()
     {
-        return CTString("[B");
+        static_assert(!std::is_array_v<ElementType>,
+                    "Traits::signature() does not handle multi-dimensional arrays");
+        return CTString("[") + Traits<U>::signature();
+    }
+
+    static constexpr auto convertFromJni(QJniObject &&object)
+    {
+        return QJniArray<ElementType>(std::move(object));
     }
 };
 }

src/corelib/kernel/qjniobject.h

@@ -20,6 +20,13 @@ namespace QtJniTypes
 {
 namespace Detail
 {
+// any type with an "jobject object()" member function stores a global reference
+template <typename T, typename = void> struct StoresGlobalRefTest : std::false_type {};
+template <typename T>
+struct StoresGlobalRefTest<T, std::void_t<decltype(std::declval<T>().object())>>
+    : std::is_same<decltype(std::declval<T>().object()), jobject>
+{};
+
 template <typename ...Args>
 struct LocalFrame {
     mutable JNIEnv *env;
@@ -39,15 +46,6 @@ struct LocalFrame {
             hasFrame = jniEnv()->PushLocalFrame(sizeof...(Args)) == 0;
         return hasFrame;
     }
-    template <typename T>
-    auto newLocalRef(jobject object)
-    {
-        if (!ensureFrame()) {
-            // if the JVM is out of memory, avoid making matters worse
-            return T{};
-        }
-        return static_cast<T>(jniEnv()->NewLocalRef(object));
-    }
     JNIEnv *jniEnv() const
     {
         if (!env)
@@ -59,9 +57,28 @@ struct LocalFrame {
         return env ? QJniEnvironment::checkAndClearExceptions(env) : false;
     }
     template <typename T>
-    auto convertToJni(T &&value);
+    auto convertToJni(T &&value)
+    {
+        using Type = q20::remove_cvref_t<T>;
+        using ResultType = decltype(QtJniTypes::Traits<Type>::convertToJni(jniEnv(),
+                                                                           std::declval<T&&>()));
+        if constexpr (std::is_base_of_v<std::remove_pointer_t<jobject>,
+                                        std::remove_pointer_t<ResultType>>) {
+            // Make sure the local frame is engaged if we create a jobject, unless
+            // we know that the value stores a global reference that it returns.
+            if constexpr (!qxp::is_detected_v<StoresGlobalRefTest, Type>) {
+                if (!ensureFrame())
+                    return ResultType{};
+            }
+        }
+        return QtJniTypes::Traits<Type>::convertToJni(jniEnv(), std::forward<T>(value));
+    }
     template <typename T>
-    auto convertFromJni(QJniObject &&object);
+    auto convertFromJni(QJniObject &&object)
+    {
+        using Type = q20::remove_cvref_t<T>;
+        return QtJniTypes::Traits<Type>::convertFromJni(std::move(object));
+    }
 };
 }
 }
@@ -833,6 +850,14 @@ private:
 template <typename T> struct Traits<JObject<T>> {
     static constexpr auto signature() { return Traits<T>::signature(); }
     static constexpr auto className() { return Traits<T>::className(); }
+    static auto convertToJni(JNIEnv *, const JObject<T> &value)
+    {
+        return value.object();
+    }
+    static auto convertFromJni(QJniObject &&object)
+    {
+        return JObject<T>(std::move(object));
+    }
 };
 
 template<>
@@ -847,87 +872,42 @@ struct Traits<QJniObject>
     {
         return CTString("Ljava/lang/Object;");
     }
+
+    static auto convertToJni(JNIEnv *, const QJniObject &value)
+    {
+        return value.object();
+    }
+    static auto convertFromJni(QJniObject &&object)
+    {
+        return std::move(object);
+    }
+};
+
+template<>
+struct Traits<QString>
+{
+    static constexpr auto className()
+    {
+        return CTString("java/lang/String");
+    }
+    static constexpr auto signature()
+    {
+        return CTString("Ljava/lang/String;");
+    }
+
+    static auto convertToJni(JNIEnv *env, const QString &value)
+    {
+        return QtJniTypes::Detail::fromQString(value, env);
+    }
+
+    static auto convertFromJni(QJniObject &&object)
+    {
+        return object.toString();
+    }
 };
 
 }
 
-// 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
-#include <QtCore/qjniarray.h>
-QT_BEGIN_NAMESPACE
-
-namespace QtJniTypes {
-namespace detail {
-template <typename C>
-using FromContainerTest = decltype(QJniArrayBase::fromContainer(std::declval<C>()));
-
-template <typename C>
-static constexpr bool isCompatibleSourceContainer = qxp::is_detected_v<FromContainerTest, C>;
-
-template <typename It>
-using IsReferenceWrapperTest = typename It::refwrapper;
-
-template <typename It>
-static constexpr bool isReferenceWrapper = qxp::is_detected_v<IsReferenceWrapperTest, It>;
-}
-}
-
-template <typename ...Args>
-template <typename T>
-auto QtJniTypes::Detail::LocalFrame<Args...>::convertToJni(T &&value)
-{
-    using Type = q20::remove_cvref_t<T>;
-    if constexpr (std::is_same_v<Type, QString>) {
-        if (ensureFrame()) // fromQString already returns a local reference
-            return QtJniTypes::Detail::fromQString(value, jniEnv());
-        return jstring{};
-    } else if constexpr (QtJniTypes::IsJniArray<Type>::value) {
-        return value.arrayObject();
-    } else if constexpr (QtJniTypes::detail::isCompatibleSourceContainer<T>) {
-        using QJniArrayType = decltype(QJniArrayBase::fromContainer(std::forward<T>(value)));
-        using ArrayType = decltype(std::declval<QJniArrayType>().arrayObject());
-        return newLocalRef<ArrayType>(QJniArrayBase::fromContainer(std::forward<T>(value)).template object<jobject>());
-    } else if constexpr (QtJniTypes::detail::isReferenceWrapper<Type>) {
-        return convertToJni(*value);
-    } else if constexpr (std::is_base_of_v<QJniObject, Type>
-                      || std::is_base_of_v<QtJniTypes::JObjectBase, Type>) {
-        return value.object();
-    } else {
-        return std::forward<T>(value);
-    }
-}
-
-template <typename ...Args>
-template <typename T>
-auto QtJniTypes::Detail::LocalFrame<Args...>::convertFromJni(QJniObject &&object)
-{
-    using Type = q20::remove_cvref_t<T>;
-    if constexpr (std::is_same_v<Type, QString>) {
-        return object.toString();
-    } else if constexpr (QtJniTypes::IsJniArray<Type>::value) {
-        return T(std::move(object));
-    } else if constexpr (QtJniTypes::detail::isCompatibleSourceContainer<Type>) {
-        // if we were to create a QJniArray from Type...
-        using QJniArrayType = decltype(QJniArrayBase::fromContainer(std::declval<Type>()));
-        // then that QJniArray would have elements of type
-        using ElementType = typename QJniArrayType::Type;
-        // construct a QJniArray from a jobject pointer of that type
-        return QJniArray<ElementType>(object.template object<jarray>()).toContainer();
-    } else if constexpr (std::is_array_v<Type>) {
-        using ElementType = std::remove_extent_t<Type>;
-        return QJniArray<ElementType>(std::move(object));
-    } else if constexpr (std::is_base_of_v<QJniObject, Type>
-                        && !std::is_same_v<QJniObject, Type>) {
-        return T{std::move(object)};
-    } else if constexpr (std::is_base_of_v<QtJniTypes::JObjectBase, Type>) {
-        return T{std::move(object)};
-    } else {
-        return std::move(object);
-    }
-}
-
-
 QT_END_NAMESPACE
 
 #endif

src/corelib/kernel/qjnitypes.h

@@ -8,6 +8,7 @@
 
 #include <QtCore/qjnitypes_impl.h>
 #include <QtCore/qjniobject.h>
+#include <QtCore/qjniarray.h>
 
 #if 0
 // This is needed for generating the QtJniTypes forward header
@@ -140,8 +141,7 @@ namespace Detail {
 template <typename Arg>
 struct JNITypeForArgImpl
 {
-    using LocalFrame = QtJniTypes::Detail::LocalFrame<void>;
+    using JNIType = decltype(QtJniTypes::Traits<Arg>::convertToJni(nullptr, {}));
-    using JNIType = decltype(std::declval<LocalFrame>().convertToJni(std::declval<Arg>()));
     static Arg fromVarArg(JNIType t) // JNIType is always POD
     {
         // Special case: if convertToJni doesn't do anything, don't do anything
@@ -150,8 +150,7 @@ struct JNITypeForArgImpl
         if constexpr (std::is_same_v<JNIType, Arg>) {
             return t;
         } else {
-            LocalFrame frame;
+            return QtJniTypes::Traits<Arg>::convertFromJni(t);
-            return frame.template convertFromJni<Arg>(t);
         }
     }
 };

src/corelib/kernel/qjnitypes_impl.h

@@ -15,6 +15,8 @@
 
 QT_BEGIN_NAMESPACE
 
+class QJniObject;
+
 namespace QtJniTypes
 {
 
@@ -164,7 +166,7 @@ template<size_t N> struct IsStringType<const char[N]> : std::true_type {};
 template<size_t N> struct IsStringType<const char(&)[N]> : std::true_type {};
 template<size_t N> struct IsStringType<char[N]> : std::true_type {};
 
-template <typename T>
+template <typename T, typename = void>
 struct Traits {
     // The return type of className/signature becomes void for any type
     // not handled here. This indicates that the Traits type is not specialized
@@ -189,11 +191,6 @@ struct Traits {
         if constexpr (!std::is_same_v<decltype(className()), void>) {
             // the type signature of any object class is L<className>;
             return CTString("L") + className() + CTString(";");
-        } else if constexpr (std::is_array_v<T>) {
-            using UnderlyingType = typename std::remove_extent_t<T>;
-            static_assert(!std::is_array_v<UnderlyingType>,
-                        "Traits::signature() does not handle multi-dimensional arrays");
-            return CTString("[") + Traits<UnderlyingType>::signature();
         } else if constexpr (std::is_same_v<T, jobjectArray>) {
             return CTString("[Ljava/lang/Object;");
         } else if constexpr (std::is_same_v<T, jbooleanArray>) {
@@ -248,11 +245,19 @@ struct Traits {
             return CTString("V");
         } else if constexpr (std::is_enum_v<T>) {
             return Traits<std::underlying_type_t<T>>::signature();
-        } else if constexpr (std::is_same_v<T, QString>) {
-            return CTString("Ljava/lang/String;");
         }
         // else: return void -> not implemented
     }
+
+    template <typename U = T>
+    static auto convertToJni(JNIEnv *, U &&value)
+    {
+        return std::forward<U>(value);
+    }
+    static auto convertFromJni(QJniObject &&object)
+    {
+        return std::move(object);
+    }
 };
 
 template <typename Have, typename Want>