QGIM: support range provided in a std::reference_wrapper

std::reference_wrapper is a good option for explicitly passing
a reference to the model range. Some users, who don't like
raw pointers, may prefer explicit semantics like
QGIM(std::ref(myModel)) instead of passing a raw ptr.

Change-Id: I28df0842d78365c980a3edea6883a3819a1c0c33
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>

src/corelib/itemmodels/qgenericitemmodel.h

@@ -124,7 +124,7 @@ class QGenericItemModelImpl : public QGenericItemModelImplBase
 {
     Q_DISABLE_COPY_MOVE(QGenericItemModelImpl)
 public:
-    using range_type = std::remove_pointer_t<std::remove_reference_t<Range>>;
+    using range_type = QGenericItemModelDetails::remove_ptr_and_ref_t<Range>;
     using row_reference = decltype(*std::begin(std::declval<range_type&>()));
     using const_row_reference = decltype(*std::cbegin(std::declval<range_type&>()));
     using row_type = std::remove_reference_t<row_reference>;

src/corelib/itemmodels/qgenericitemmodel_impl.h

@@ -29,6 +29,34 @@ QT_BEGIN_NAMESPACE
 
 namespace QGenericItemModelDetails
 {
+    template <typename T, size_t N> static decltype(auto) refTo(T (&t)[N]) { return t; }
+    template <typename T> static T&& refTo(T&& t) { return std::forward<T>(t); }
+    template <typename T> static T& refTo(std::reference_wrapper<T> t) { return t.get(); }
+    // template <typename T> static auto refTo(T& t) -> decltype(*t) { return *t; }
+
+    template <typename T> static auto pointerTo(T *t) { return t; }
+    template <typename T> static auto pointerTo(T &t) { return std::addressof(refTo(t)); }
+    template <typename T> static auto pointerTo(const T &&t) = delete;
+
+    template <typename It>
+    auto key(It&& it) -> decltype(it.key()) { return it.key(); }
+    template <typename It>
+    auto key(It&& it) -> decltype((it->first)) { return it->first; }
+
+    template <typename It>
+    auto value(It&& it) -> decltype(it.value()) { return it.value(); }
+    template <typename It>
+    auto value(It&& it) -> decltype((it->second)) { return it->second; }
+
+    template <typename T>
+    static constexpr bool isValid(const T &t) noexcept
+    {
+        if constexpr (std::is_constructible_v<bool, T>)
+            return bool(t);
+        else
+            return true;
+    }
+
     // Test if a type is a range, and whether we can modify it using the
     // standard C++ container member functions insert, erase, and resize.
     // For the sake of QAIM, we cannot modify a range if it holds const data
@@ -135,11 +163,14 @@ namespace QGenericItemModelDetails
     template <typename T> struct range_traits<const T *> : iterable_value<Mutable::No> {};
     template <> struct range_traits<QLatin1StringView> : iterable_value<Mutable::No> {};
 
+    template <typename T>
+    using remove_ptr_and_ref_t =
+        std::remove_pointer_t<std::remove_reference_t<decltype(refTo(std::declval<T&>()))>>;
+
     template <typename C>
     [[maybe_unused]] static constexpr bool is_range_v = range_traits<C>();
     template <typename CC>
-    using if_is_range = std::enable_if_t<
+    using if_is_range = std::enable_if_t<is_range_v<remove_ptr_and_ref_t<CC>>, bool>;
-                        is_range_v<std::remove_pointer_t<std::remove_reference_t<CC>>>, bool>;
 
     // Find out how many fixed elements can be retrieved from a row element.
     // main template for simple values and ranges. Specializing for ranges
@@ -193,43 +224,13 @@ namespace QGenericItemModelDetails
     [[maybe_unused]] static constexpr int static_size_v =
                             row_traits<q20::remove_cvref_t<std::remove_pointer_t<T>>>::static_size;
 
-    template <typename T> static auto pointerTo(T *t) { return t; }
-    template <typename T> static auto pointerTo(T &t) { return std::addressof(t); }
-    template <typename T> static auto pointerTo(const T &&t) = delete;
-
-    template <typename T>
-    static constexpr bool isValid(T &&t) noexcept
-    {
-        if constexpr (std::is_constructible_v<bool, T>)
-            return bool(t);
-        else
-            return true;
-    }
-
-    template <typename It>
-    auto key(It&& it) -> decltype(it.key()) { return it.key(); }
-
-    template <typename It>
-    auto key(It&& it) -> decltype((it->first) /*pars for ref type*/ ) { return it->first; }
-
-    template <typename It>
-    auto value(It&& it) -> decltype(it.value()) { return it.value(); }
-
-    template <typename It>
-    auto value(It&& it) -> decltype((it->second)) { return it->second; }
-
     // The storage of the model data. We might store it as a pointer, or as a
     // (copied- or moved-into) value. But we always return a pointer.
     template <typename ModelStorage>
     struct ModelData
     {
-        using ModelPtr = std::conditional_t<std::is_pointer_v<ModelStorage>,
+        auto model() { return pointerTo(m_model); }
-                                            ModelStorage, ModelStorage *>;
+        auto model() const { return pointerTo(m_model); }
-        using ConstModelPtr = std::conditional_t<std::is_pointer_v<ModelStorage>,
-                                            const ModelStorage, const ModelStorage *>;
-
-        ModelPtr model() { return pointerTo(m_model); }
-        ConstModelPtr model() const { return pointerTo(m_model); }
 
         ModelStorage m_model;
     };

tests/auto/corelib/itemmodels/qgenericitemmodel/tst_qgenericitemmodel.cpp

@@ -345,6 +345,18 @@ Q_DECLARE_OPERATORS_FOR_FLAGS(tst_QGenericItemModel::ChangeActions)
 
 using Factory = std::function<std::unique_ptr<QAbstractItemModel>()>;
 
+// Pointer- and reference-tests will modify the data structure that lives in
+// m_data, so we have to keep backup copies of that data.
+template <typename T, std::enable_if_t<std::is_copy_assignable_v<T>, bool> = true>
+void createBackup(QObject* object, T& model) {
+    QObject::connect(object, &QObject::destroyed, object, [backup = model, &model]() mutable {
+        model = backup;
+    });
+}
+
+template <typename T, std::enable_if_t<!std::is_copy_assignable_v<T>, bool> = true>
+void createBackup(QObject* , T& ) {}
+
 void tst_QGenericItemModel::createTestData()
 {
     m_data.reset(new Data);
@@ -354,116 +366,78 @@ void tst_QGenericItemModel::createTestData()
     QTest::addColumn<int>("expectedColumnCount");
     QTest::addColumn<ChangeActions>("changeActions");
 
-    Factory factory;
+#define ADD_HELPER(Model, Tag, Ref, ColumnCount, Actions) \
-
+    { \
-#define ADD_HELPER(Model, Tag, Ref) \
+        Factory factory = [this]() -> std::unique_ptr<QAbstractItemModel> { \
-    factory = [this]() -> std::unique_ptr<QAbstractItemModel> { \
+            auto result = std::make_unique<QGenericItemModel>(Ref(m_data->Model)); \
-        return std::unique_ptr<QAbstractItemModel>(new QGenericItemModel(Ref->Model)); \
+            createBackup(result.get(), m_data->Model); \
+            return result; \
         }; \
-    QTest::addRow(#Model #Tag) << factory << int(std::size(m_data->Model)) \
+        QTest::addRow(#Model #Tag) << std::move(factory) << int(std::size(m_data->Model)) \
+                                   << int(ColumnCount) << ChangeActions(Actions); \
+    }
 
-#define ADD_POINTER(Model) \
+#define ADD_POINTER(Model, ColumnCount, Actions) ADD_HELPER(Model, Pointer, &, ColumnCount, Actions)
-    ADD_HELPER(Model, Pointer, &m_data) \
+#define ADD_COPY(Model, ColumnCount, Actions) ADD_HELPER(Model, Copy, *&, ColumnCount, Actions)
+#define ADD_REF(Model, ColumnCount, Actions) ADD_HELPER(Model, Ref, std::ref, ColumnCount, Actions)
+#define ADD_ALL(Model, ColumnCount, Actions) \
+    ADD_COPY(Model, ColumnCount, Actions) \
+    ADD_POINTER(Model, ColumnCount, Actions) \
+    ADD_REF(Model, ColumnCount, Actions)
 
-#define ADD_COPY(Model) \
-    ADD_HELPER(Model, Copy, m_data) \
-
-    // POINTER-tests will modify the data structure that lives in m_data,
-    // so we have to run tests on copies of that data first for each type,
-    // or only run POINTER-tests.
     // The entire test data is recreated for each test function, but test
     // functions must not change data structures other than the one tested.
 
-    ADD_COPY(fixedArrayOfNumbers)
+    ADD_ALL(fixedArrayOfNumbers, 1, ChangeAction::SetData);
-        << 1 << ChangeActions(ChangeAction::SetData);
-    ADD_POINTER(fixedArrayOfNumbers)
-        << 1 << ChangeActions(ChangeAction::SetData);
-    ADD_POINTER(cArrayOfNumbers)
-        << 1 << ChangeActions(ChangeAction::SetData);
 
-    ADD_POINTER(cArrayFixedColumns)
+    ADD_POINTER(cArrayOfNumbers, 1, ChangeAction::SetData);
-        << int(std::tuple_size_v<Row>) << (ChangeAction::SetData | ChangeAction::SetItemData);
 
-    ADD_COPY(vectorOfFixedColumns)
+    ADD_POINTER(cArrayFixedColumns,
-        << 2 << (ChangeAction::ChangeRows | ChangeAction::SetData);
+                std::tuple_size_v<Row>,
-    ADD_POINTER(vectorOfFixedColumns)
+                ChangeAction::SetData | ChangeAction::SetItemData);
-        << 2 << (ChangeAction::ChangeRows | ChangeAction::SetData);
-    ADD_COPY(vectorOfArrays)
-        << 10 << (ChangeAction::ChangeRows | ChangeAction::SetData);
-    ADD_POINTER(vectorOfArrays)
-        << 10 << (ChangeAction::ChangeRows | ChangeAction::SetData);
-    ADD_COPY(vectorOfStructs)
-        << int(std::tuple_size_v<Row>) << (ChangeAction::ChangeRows | ChangeAction::SetData
-                                                                    | ChangeAction::SetItemData);
-    ADD_POINTER(vectorOfStructs)
-        << int(std::tuple_size_v<Row>) << (ChangeAction::ChangeRows | ChangeAction::SetData
-                                                                    | ChangeAction::SetItemData);
-    ADD_COPY(vectorOfConstStructs)
-        << int(std::tuple_size_v<ConstRow>) << ChangeActions(ChangeAction::ChangeRows);
-    ADD_POINTER(vectorOfConstStructs)
-        << int(std::tuple_size_v<ConstRow>) << ChangeActions(ChangeAction::ChangeRows);
 
-    ADD_COPY(vectorOfGadgets)
+    ADD_ALL(vectorOfFixedColumns, 2, ChangeAction::ChangeRows | ChangeAction::SetData);
-        << 3 << (ChangeAction::ChangeRows | ChangeAction::SetData | ChangeAction::SetItemData);
+
-    ADD_POINTER(vectorOfGadgets)
+    ADD_ALL(vectorOfArrays, 10, ChangeAction::ChangeRows | ChangeAction::SetData);
-        << 3 << (ChangeAction::ChangeRows | ChangeAction::SetData | ChangeAction::SetItemData);
+
-    ADD_COPY(listOfGadgets)
+    ADD_ALL(vectorOfStructs,
-        << 1 << (ChangeAction::ChangeRows | ChangeAction::SetData | ChangeAction::SetItemData);
+            std::tuple_size_v<Row>,
-    ADD_POINTER(listOfGadgets)
+            ChangeAction::ChangeRows | ChangeAction::SetData | ChangeAction::SetItemData);
-        << 1 << (ChangeAction::ChangeRows | ChangeAction::SetData | ChangeAction::SetItemData);
+
-    ADD_COPY(listOfObjects)
+    ADD_ALL(vectorOfConstStructs, std::tuple_size_v<ConstRow>, ChangeAction::ChangeRows);
-        << 2 << (ChangeAction::ChangeRows | ChangeAction::SetData);
+
-    ADD_COPY(listOfMetaObjectTuple)
+    ADD_ALL(vectorOfGadgets, 3, ChangeAction::ChangeRows | ChangeAction::SetData | ChangeAction::SetItemData);
-        << 1 << (ChangeAction::ChangeRows | ChangeAction::SetData | ChangeAction::SetItemData);
+
-    ADD_COPY(tableOfMetaObjectTuple)
+    ADD_ALL(listOfGadgets, 1, ChangeAction::ChangeRows | ChangeAction::SetData | ChangeAction::SetItemData);
-        << 2 << (ChangeAction::ChangeRows | ChangeAction::SetData);
+
+    ADD_COPY(listOfObjects, 2, ChangeAction::ChangeRows | ChangeAction::SetData);
+
+    ADD_ALL(tableOfNumbers, 5, ChangeAction::All);
 
-    ADD_COPY(tableOfNumbers)
-        << 5 << ChangeActions(ChangeAction::All);
-    ADD_POINTER(tableOfNumbers)
-        << 5 << ChangeActions(ChangeAction::All);
     // only adding as pointer, copy would operate on the same data
-    ADD_POINTER(tableOfPointers)
+    ADD_POINTER(tableOfPointers, 2, ChangeAction::All | ChangeAction::SetItemData);
-        << 2 << ChangeActions(ChangeAction::All | ChangeAction::SetItemData);
+    ADD_POINTER(tableOfRowPointers,
-    ADD_POINTER(tableOfRowPointers)
+                std::tuple_size_v<Row>,
-        << int(std::tuple_size_v<Row>) << (ChangeAction::ChangeRows | ChangeAction::SetData
+                ChangeAction::ChangeRows | ChangeAction::SetData | ChangeAction::SetItemData);
-                                                                    | ChangeAction::SetItemData);
 
-    ADD_COPY(arrayOfConstNumbers)
+    ADD_ALL(arrayOfConstNumbers, 1, ChangeAction::ReadOnly);
-        << 1 << ChangeActions(ChangeAction::ReadOnly);
-    ADD_POINTER(arrayOfConstNumbers)
-        << 1 << ChangeActions(ChangeAction::ReadOnly);
 
-    ADD_COPY(constListOfNumbers)
+    ADD_ALL(constListOfNumbers, 1, ChangeAction::ReadOnly);
-        << 1 << ChangeActions(ChangeAction::ReadOnly);
-    ADD_POINTER(constListOfNumbers)
-        << 1 << ChangeActions(ChangeAction::ReadOnly);
 
-    ADD_COPY(constTableOfNumbers)
+    ADD_ALL(constTableOfNumbers, 5, ChangeAction::ReadOnly);
-        << 5 << ChangeActions(ChangeAction::ReadOnly);
-    ADD_POINTER(constTableOfNumbers)
-        << 5 << ChangeActions(ChangeAction::ReadOnly);
 
-    ADD_COPY(listOfNamedRoles)
+    ADD_ALL(listOfNamedRoles, 1, ChangeAction::ChangeRows | ChangeAction::SetData | ChangeAction::SetItemData);
-        << 1 << (ChangeAction::ChangeRows | ChangeAction::SetData | ChangeAction::SetItemData);
+
-    ADD_POINTER(listOfNamedRoles)
+    ADD_ALL(tableOfEnumRoles, 1, ChangeAction::All | ChangeAction::SetItemData);
-        << 1 << (ChangeAction::ChangeRows | ChangeAction::SetData | ChangeAction::SetItemData);
+
-    ADD_COPY(tableOfEnumRoles)
+    ADD_ALL(tableOfIntRoles, 1, ChangeAction::All | ChangeAction::SetItemData);
-        << 1 << ChangeActions(ChangeAction::All | ChangeAction::SetItemData);
+
-    ADD_POINTER(tableOfEnumRoles)
+    ADD_ALL(stdTableOfIntRoles, 1, ChangeAction::All | ChangeAction::SetItemData);
-        << 1 << ChangeActions(ChangeAction::All | ChangeAction::SetItemData);
-    ADD_COPY(tableOfIntRoles)
-        << 1 << ChangeActions(ChangeAction::All | ChangeAction::SetItemData);
-    ADD_POINTER(tableOfIntRoles)
-        << 1 << ChangeActions(ChangeAction::All | ChangeAction::SetItemData);
-    ADD_COPY(stdTableOfIntRoles)
-        << 1 << ChangeActions(ChangeAction::All | ChangeAction::SetItemData);
-    ADD_POINTER(stdTableOfIntRoles)
-        << 1 << ChangeActions(ChangeAction::All | ChangeAction::SetItemData);
 
 #undef ADD_COPY
 #undef ADD_POINTER
 #undef ADD_HELPER
+#undef ADD_ALL
 
     QTest::addRow("Moved table") << Factory([]{
         QList<std::vector<QString>> movedTable = {
@@ -728,14 +702,18 @@ void tst_QGenericItemModel::insertRows()
              changeActions.testFlag(ChangeAction::InsertRows));
 
     auto ignoreFailureFromAssociativeContainers = [] {
-        QEXPECT_FAIL("listOfNamedRolesPointer", "QVariantMap is empty by design", Continue);
+        for (auto suffix : { "Pointer", "Copy", "Ref" }) {
-        QEXPECT_FAIL("listOfNamedRolesCopy", "QVariantMap is empty by design", Continue);
+            auto addCase = [suffix](const std::string& testName,
-        QEXPECT_FAIL("tableOfEnumRolesPointer", "QVariantMap is empty by design", Continue);
+                                    const std::string& containerName) {
-        QEXPECT_FAIL("tableOfEnumRolesCopy", "QVariantMap is empty by design", Continue);
+              QEXPECT_FAIL((testName + suffix).c_str(),
-        QEXPECT_FAIL("tableOfIntRolesPointer", "QVariantMap is empty by design", Continue);
+                           (containerName + " is empty by design").c_str(),
-        QEXPECT_FAIL("tableOfIntRolesCopy", "QVariantMap is empty by design", Continue);
+                           Continue);
-        QEXPECT_FAIL("stdTableOfIntRolesPointer", "std::map is empty by design", Continue);
+            };
-        QEXPECT_FAIL("stdTableOfIntRolesCopy", "std::map is empty by design", Continue);
+            addCase("listOfNamedRoles", "QVariantMap");
+            addCase("tableOfEnumRoles", "QVariantMap");
+            addCase("tableOfIntRoles", "QVariantMap");
+            addCase("stdTableOfIntRoles", "std::map");
+        }
     };
     // get and put data into the new row
     const QModelIndex firstItem = model->index(0, 0);