JNI: Improve the constraint on QJniArray::fromContainer

We can create a QJniArray from any container that has a forward
iterator. A contiguous container can be used to optimize the code path,
as long as the element type is primitive (i.e. not an object type).

Fixes: QTBUG-126151
Change-Id: I21915f944d226d6d4f1113a54e5602ddc9cd727e
Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>
(cherry picked from commit 71be7834e67216010dc74ed855dc7b513e302f1a)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

src/corelib/kernel/qjniarray.h

@@ -151,6 +151,28 @@ private:
 class QJniArrayBase
 {
     // for SFINAE'ing out the fromContainer named constructor
+    template <typename C, typename = void> struct IsSequentialContainerHelper : std::false_type
+    {
+        static constexpr bool isForwardIterable = false;
+    };
+    template <typename C>
+    struct IsSequentialContainerHelper<C, std::void_t<typename std::iterator_traits<typename C::const_iterator>::iterator_category,
+                                                      typename C::value_type,
+                                                      decltype(std::size(std::declval<C>()))
+                                                     >
+                                      > : std::true_type
+    {
+        static constexpr bool isForwardIterable = std::is_convertible_v<
+            typename std::iterator_traits<typename C::const_iterator>::iterator_category,
+            std::forward_iterator_tag
+        >;
+    };
+    template <>
+    struct IsSequentialContainerHelper<QByteArray, void>
+    {
+        static constexpr bool isForwardIterable = true;
+    };
+
     template <typename C, typename = void> struct IsContiguousContainerHelper : std::false_type {};
     template <typename C>
     struct IsContiguousContainerHelper<C, std::void_t<decltype(std::data(std::declval<C>())),
@@ -161,6 +183,23 @@ class QJniArrayBase
 
 protected:
     // these are used in QJniArray
+    template <typename C, typename = void>
+    struct ElementTypeHelper
+    {
+        static constexpr bool isObject = false;
+        static constexpr bool isPrimitive = false;
+    };
+    template <typename C>
+    struct ElementTypeHelper<C, std::void_t<typename C::value_type>>
+    {
+        using E = typename C::value_type;
+        static constexpr bool isObject = QtJniTypes::isObjectType<E>();
+        static constexpr bool isPrimitive = QtJniTypes::isPrimitiveType<E>();
+    };
+
+    template <typename CRef, typename C = q20::remove_cvref_t<CRef>>
+    static constexpr bool isContiguousContainer = IsContiguousContainerHelper<C>::value;
+
     template <typename From, typename To>
     using if_convertible = std::enable_if_t<QtPrivate::AreArgumentsConvertibleWithoutNarrowingBase<From, To>::value, bool>;
     template <typename From, typename To>
@@ -184,12 +223,21 @@ public:
         return 0;
     }
 
-    template <typename C>
+    // We can create an array from any forward-iterable container, and optimize
-    static constexpr bool isContiguousContainer = IsContiguousContainerHelper<q20::remove_cvref_t<C>>::value;
+    // for contiguous containers holding primitive elements. QJniArray is a
-    template <typename C>
+    // forward-iterable container, so explicitly remove that from the overload
-    using if_contiguous_container = std::enable_if_t<isContiguousContainer<C>, bool>;
+    // set so that the copy constructors get used instead.
+    // Used also in the deduction guide, so must be public
+    template <typename CRef, typename C = q20::remove_cvref_t<CRef>>
+    static constexpr bool isCompatibleSourceContainer =
+        (IsSequentialContainerHelper<C>::isForwardIterable
+         || (isContiguousContainer<C> && ElementTypeHelper<C>::isPrimitive))
+        && !std::is_base_of_v<QJniArrayBase, C>;
 
-    template <typename Container, if_contiguous_container<Container> = true>
+    template <typename C>
+    using if_compatible_source_container = std::enable_if_t<isCompatibleSourceContainer<C>, bool>;
+
+    template <typename Container, if_compatible_source_container<Container> = true>
     static auto fromContainer(Container &&container)
     {
         Q_ASSERT_X(size_t(std::size(container)) <= size_t((std::numeric_limits<size_type>::max)()),
@@ -337,7 +385,7 @@ public:
     template <typename Other, unless_convertible<Other, T> = true>
     QJniArray(QJniArray<Other> &&other) noexcept = delete;
 
-    template <typename Container, if_contiguous_container<Container> = true>
+    template <typename Container, if_compatible_source_container<Container> = true>
     explicit QJniArray(Container &&container)
         : QJniArrayBase(QJniArrayBase::fromContainer(std::forward<Container>(container)))
     {
@@ -453,7 +501,7 @@ public:
         } else if constexpr (std::is_base_of_v<std::remove_pointer_t<jobject>, std::remove_pointer_t<T>>) {
             for (auto element : *this)
                 container.emplace_back(element);
-        } else if constexpr (QJniArrayBase::isContiguousContainer<ContainerType>) {
+        } else if constexpr (isContiguousContainer<ContainerType>) {
             container.resize(sz);
             if constexpr (QtJniTypes::sameTypeForJni<T, jbyte>) {
                 env->GetByteArrayRegion(object<jbyteArray>(),
@@ -496,7 +544,7 @@ public:
 // fromContainer() maps several C++ types to the same JNI type (e.g. both jboolean and
 // bool become QJniArray<jboolean>), we have to deduce to what fromContainer() would
 // give us.
-template <typename Container, QJniArrayBase::if_contiguous_container<Container> = true>
+template <typename Container, QJniArrayBase::if_compatible_source_container<Container> = true>
 QJniArray(Container) -> QJniArray<typename decltype(QJniArrayBase::fromContainer(std::declval<Container>()))::value_type>;
 
 template <typename ElementType, typename List, typename NewFn, typename SetFn>
@@ -508,10 +556,16 @@ auto QJniArrayBase::makeArray(List &&list, NewFn &&newArray, SetFn &&setRegion)
     if (QJniEnvironment::checkAndClearExceptions(env))
         return QJniArray<ElementType>();
 
-    // can't use static_cast here because we have signed/unsigned mismatches
     if (length) {
+        // can't use static_cast here because we have signed/unsigned mismatches
+        if constexpr (isContiguousContainer<List>) {
             (env->*setRegion)(localArray, 0, length,
                             reinterpret_cast<const ElementType *>(std::data(std::as_const(list))));
+        } else {
+            size_type i = 0;
+            for (const auto &e : std::as_const(list))
+                (env->*setRegion)(localArray, i++, 1, reinterpret_cast<const ElementType *>(&e));
+        }
     }
     return QJniArray<ElementType>(localArray);
 };

src/corelib/kernel/qjniarray.qdoc

@@ -16,15 +16,15 @@
 */
 
 /*!
-    \fn template <typename Container, QJniArrayBase::if_contiguous_container<Container>> auto QJniArrayBase::fromContainer(Container &&container)
+    \fn template <typename Container, QJniArrayBase::if_compatible_source_container<Container>> auto QJniArrayBase::fromContainer(Container &&container)
 
     Creates a Java array holding the data in \a container, and returns a
     QJniArray instance that wraps it.
 
-//! [contiguous-containers]
+//! [forward-iterable-containers]
     This function only participates in overload resolution if \c{Container}
-    is a contiguous container storing elements of a \l{JNI types}{JNI type}
+    is a container that stores elements of a \l{JNI types}{JNI type} or equivalent
-    or equivalent C++ type.
+    C++ type, and provides a forward iterator.
 
     The specialization of the constructed QJniArray depends on the value type
     of the \a container. For a \c{Container<T>} (such as e.g. \c{QList<T>}) it
@@ -47,7 +47,7 @@
     \li QJniObject
     \li QJniArray<jobject>
     \endtable
-//! [contiguous-containers]
+//! [forward-iterable-containers]
 
     \sa QJniArray::toContainer()
 */
@@ -278,13 +278,13 @@
 */
 
 /*!
-    \fn template <typename T> template <typename Container, QJniArrayBase::if_contiguous_container<Container>> QJniArray<T>::QJniArray(Container &&container)
+    \fn template <typename T> template <typename Container, QJniArrayBase::if_compatible_source_container<Container>> QJniArray<T>::QJniArray(Container &&container)
 
     Constructs a QJniArray that wraps a newly-created Java array for elements of
     type \c{Container::value_type}, and populates the Java array with the data from
     \a container.
 
-    \include qjniarray.qdoc contiguous-containers
+    \include qjniarray.qdoc forward-iterable-containers
 
     \sa QJniArrayBase::fromContainer(), toContainer()
 */

src/corelib/kernel/qjniobject.h

@@ -848,6 +848,16 @@ 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 ...Args>
 template <typename T>
 auto QJniObject::LocalFrame<Args...>::convertToJni(T &&value)
@@ -857,7 +867,7 @@ auto QJniObject::LocalFrame<Args...>::convertToJni(T &&value)
         return newLocalRef<jstring>(QJniObject::fromString(value));
     } else if constexpr (QtJniTypes::IsJniArray<Type>::value) {
         return value.arrayObject();
-    } else if constexpr (QJniArrayBase::isContiguousContainer<T>) {
+    } 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>());
@@ -878,7 +888,7 @@ auto QJniObject::LocalFrame<Args...>::convertFromJni(QJniObject &&object)
         return object.toString();
     } else if constexpr (QtJniTypes::IsJniArray<Type>::value) {
         return T(std::move(object));
-    } else if constexpr (QJniArrayBase::isContiguousContainer<Type>) {
+    } 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

tests/auto/corelib/kernel/qjniarray/tst_qjniarray.cpp

@@ -182,6 +182,18 @@ void tst_QJniArray::construct()
         QJniArray<jint> list{QList<int>{1, 2, 3}};
         QCOMPARE(list.size(), 3);
     }
+
+    // non-contiguous container
+    {
+       std::list list({1, 2, 3});
+       QJniArray array(list);
+       QCOMPARE(array.size(), 3);
+    }
+    {
+        std::list list({QString(), QString(), QString()});
+        QJniArray array(list);
+        QCOMPARE(array.size(), 3);
+    }
 }
 
 // Verify that we can convert QJniArrays into each other as long as element types