From 7803e6c000cc0fddba392bcbaefdc3a93ff4b26c Mon Sep 17 00:00:00 2001 From: Artem Dyomin Date: Sun, 23 Mar 2025 22:25:18 +0100 Subject: [PATCH] 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 --- src/corelib/itemmodels/qgenericitemmodel.h | 2 +- .../itemmodels/qgenericitemmodel_impl.h | 69 +++---- .../tst_qgenericitemmodel.cpp | 180 ++++++++---------- 3 files changed, 115 insertions(+), 136 deletions(-) diff --git a/src/corelib/itemmodels/qgenericitemmodel.h b/src/corelib/itemmodels/qgenericitemmodel.h index 15afcecd701..30da0e84e49 100644 --- a/src/corelib/itemmodels/qgenericitemmodel.h +++ b/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>; + using range_type = QGenericItemModelDetails::remove_ptr_and_ref_t; using row_reference = decltype(*std::begin(std::declval())); using const_row_reference = decltype(*std::cbegin(std::declval())); using row_type = std::remove_reference_t; diff --git a/src/corelib/itemmodels/qgenericitemmodel_impl.h b/src/corelib/itemmodels/qgenericitemmodel_impl.h index 1048df779e1..8f986a2a702 100644 --- a/src/corelib/itemmodels/qgenericitemmodel_impl.h +++ b/src/corelib/itemmodels/qgenericitemmodel_impl.h @@ -29,6 +29,34 @@ QT_BEGIN_NAMESPACE namespace QGenericItemModelDetails { + template static decltype(auto) refTo(T (&t)[N]) { return t; } + template static T&& refTo(T&& t) { return std::forward(t); } + template static T& refTo(std::reference_wrapper t) { return t.get(); } + // template static auto refTo(T& t) -> decltype(*t) { return *t; } + + template static auto pointerTo(T *t) { return t; } + template static auto pointerTo(T &t) { return std::addressof(refTo(t)); } + template static auto pointerTo(const T &&t) = delete; + + template + auto key(It&& it) -> decltype(it.key()) { return it.key(); } + template + auto key(It&& it) -> decltype((it->first)) { return it->first; } + + template + auto value(It&& it) -> decltype(it.value()) { return it.value(); } + template + auto value(It&& it) -> decltype((it->second)) { return it->second; } + + template + static constexpr bool isValid(const T &t) noexcept + { + if constexpr (std::is_constructible_v) + 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 struct range_traits : iterable_value {}; template <> struct range_traits : iterable_value {}; + template + using remove_ptr_and_ref_t = + std::remove_pointer_t()))>>; + template [[maybe_unused]] static constexpr bool is_range_v = range_traits(); template - using if_is_range = std::enable_if_t< - is_range_v>>, bool>; + using if_is_range = std::enable_if_t>, 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>>::static_size; - template static auto pointerTo(T *t) { return t; } - template static auto pointerTo(T &t) { return std::addressof(t); } - template static auto pointerTo(const T &&t) = delete; - - template - static constexpr bool isValid(T &&t) noexcept - { - if constexpr (std::is_constructible_v) - return bool(t); - else - return true; - } - - template - auto key(It&& it) -> decltype(it.key()) { return it.key(); } - - template - auto key(It&& it) -> decltype((it->first) /*pars for ref type*/ ) { return it->first; } - - template - auto value(It&& it) -> decltype(it.value()) { return it.value(); } - - template - 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 struct ModelData { - using ModelPtr = std::conditional_t, - ModelStorage, ModelStorage *>; - using ConstModelPtr = std::conditional_t, - const ModelStorage, const ModelStorage *>; - - ModelPtr model() { return pointerTo(m_model); } - ConstModelPtr model() const { return pointerTo(m_model); } + auto model() { return pointerTo(m_model); } + auto model() const { return pointerTo(m_model); } ModelStorage m_model; }; diff --git a/tests/auto/corelib/itemmodels/qgenericitemmodel/tst_qgenericitemmodel.cpp b/tests/auto/corelib/itemmodels/qgenericitemmodel/tst_qgenericitemmodel.cpp index c30264d01b7..10d1fb34ab6 100644 --- a/tests/auto/corelib/itemmodels/qgenericitemmodel/tst_qgenericitemmodel.cpp +++ b/tests/auto/corelib/itemmodels/qgenericitemmodel/tst_qgenericitemmodel.cpp @@ -345,6 +345,18 @@ Q_DECLARE_OPERATORS_FOR_FLAGS(tst_QGenericItemModel::ChangeActions) using Factory = std::function()>; +// 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 , bool> = true> +void createBackup(QObject* object, T& model) { + QObject::connect(object, &QObject::destroyed, object, [backup = model, &model]() mutable { + model = backup; + }); +} + +template , 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("expectedColumnCount"); QTest::addColumn("changeActions"); - Factory factory; +#define ADD_HELPER(Model, Tag, Ref, ColumnCount, Actions) \ + { \ + Factory factory = [this]() -> std::unique_ptr { \ + auto result = std::make_unique(Ref(m_data->Model)); \ + createBackup(result.get(), m_data->Model); \ + return result; \ + }; \ + QTest::addRow(#Model #Tag) << std::move(factory) << int(std::size(m_data->Model)) \ + << int(ColumnCount) << ChangeActions(Actions); \ + } -#define ADD_HELPER(Model, Tag, Ref) \ - factory = [this]() -> std::unique_ptr { \ - return std::unique_ptr(new QGenericItemModel(Ref->Model)); \ - }; \ - QTest::addRow(#Model #Tag) << factory << int(std::size(m_data->Model)) \ +#define ADD_POINTER(Model, ColumnCount, Actions) ADD_HELPER(Model, Pointer, &, ColumnCount, Actions) +#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_POINTER(Model) \ - ADD_HELPER(Model, Pointer, &m_data) \ - -#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) - << 1 << ChangeActions(ChangeAction::SetData); - ADD_POINTER(fixedArrayOfNumbers) - << 1 << ChangeActions(ChangeAction::SetData); - ADD_POINTER(cArrayOfNumbers) - << 1 << ChangeActions(ChangeAction::SetData); + ADD_ALL(fixedArrayOfNumbers, 1, ChangeAction::SetData); - ADD_POINTER(cArrayFixedColumns) - << int(std::tuple_size_v) << (ChangeAction::SetData | ChangeAction::SetItemData); + ADD_POINTER(cArrayOfNumbers, 1, ChangeAction::SetData); - ADD_COPY(vectorOfFixedColumns) - << 2 << (ChangeAction::ChangeRows | ChangeAction::SetData); - ADD_POINTER(vectorOfFixedColumns) - << 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) << (ChangeAction::ChangeRows | ChangeAction::SetData - | ChangeAction::SetItemData); - ADD_POINTER(vectorOfStructs) - << int(std::tuple_size_v) << (ChangeAction::ChangeRows | ChangeAction::SetData - | ChangeAction::SetItemData); - ADD_COPY(vectorOfConstStructs) - << int(std::tuple_size_v) << ChangeActions(ChangeAction::ChangeRows); - ADD_POINTER(vectorOfConstStructs) - << int(std::tuple_size_v) << ChangeActions(ChangeAction::ChangeRows); + ADD_POINTER(cArrayFixedColumns, + std::tuple_size_v, + ChangeAction::SetData | ChangeAction::SetItemData); - ADD_COPY(vectorOfGadgets) - << 3 << (ChangeAction::ChangeRows | ChangeAction::SetData | ChangeAction::SetItemData); - ADD_POINTER(vectorOfGadgets) - << 3 << (ChangeAction::ChangeRows | ChangeAction::SetData | ChangeAction::SetItemData); - ADD_COPY(listOfGadgets) - << 1 << (ChangeAction::ChangeRows | ChangeAction::SetData | ChangeAction::SetItemData); - ADD_POINTER(listOfGadgets) - << 1 << (ChangeAction::ChangeRows | ChangeAction::SetData | ChangeAction::SetItemData); - ADD_COPY(listOfObjects) - << 2 << (ChangeAction::ChangeRows | ChangeAction::SetData); - ADD_COPY(listOfMetaObjectTuple) - << 1 << (ChangeAction::ChangeRows | ChangeAction::SetData | ChangeAction::SetItemData); - ADD_COPY(tableOfMetaObjectTuple) - << 2 << (ChangeAction::ChangeRows | ChangeAction::SetData); + ADD_ALL(vectorOfFixedColumns, 2, ChangeAction::ChangeRows | ChangeAction::SetData); + + ADD_ALL(vectorOfArrays, 10, ChangeAction::ChangeRows | ChangeAction::SetData); + + ADD_ALL(vectorOfStructs, + std::tuple_size_v, + ChangeAction::ChangeRows | ChangeAction::SetData | ChangeAction::SetItemData); + + ADD_ALL(vectorOfConstStructs, std::tuple_size_v, ChangeAction::ChangeRows); + + ADD_ALL(vectorOfGadgets, 3, ChangeAction::ChangeRows | ChangeAction::SetData | ChangeAction::SetItemData); + + ADD_ALL(listOfGadgets, 1, ChangeAction::ChangeRows | ChangeAction::SetData | ChangeAction::SetItemData); + + 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) - << 2 << ChangeActions(ChangeAction::All | ChangeAction::SetItemData); - ADD_POINTER(tableOfRowPointers) - << int(std::tuple_size_v) << (ChangeAction::ChangeRows | ChangeAction::SetData - | ChangeAction::SetItemData); + ADD_POINTER(tableOfPointers, 2, ChangeAction::All | ChangeAction::SetItemData); + ADD_POINTER(tableOfRowPointers, + std::tuple_size_v, + ChangeAction::ChangeRows | ChangeAction::SetData | ChangeAction::SetItemData); - ADD_COPY(arrayOfConstNumbers) - << 1 << ChangeActions(ChangeAction::ReadOnly); - ADD_POINTER(arrayOfConstNumbers) - << 1 << ChangeActions(ChangeAction::ReadOnly); + ADD_ALL(arrayOfConstNumbers, 1, ChangeAction::ReadOnly); - ADD_COPY(constListOfNumbers) - << 1 << ChangeActions(ChangeAction::ReadOnly); - ADD_POINTER(constListOfNumbers) - << 1 << ChangeActions(ChangeAction::ReadOnly); + ADD_ALL(constListOfNumbers, 1, ChangeAction::ReadOnly); - ADD_COPY(constTableOfNumbers) - << 5 << ChangeActions(ChangeAction::ReadOnly); - ADD_POINTER(constTableOfNumbers) - << 5 << ChangeActions(ChangeAction::ReadOnly); + ADD_ALL(constTableOfNumbers, 5, ChangeAction::ReadOnly); - ADD_COPY(listOfNamedRoles) - << 1 << (ChangeAction::ChangeRows | ChangeAction::SetData | ChangeAction::SetItemData); - ADD_POINTER(listOfNamedRoles) - << 1 << (ChangeAction::ChangeRows | ChangeAction::SetData | ChangeAction::SetItemData); - ADD_COPY(tableOfEnumRoles) - << 1 << ChangeActions(ChangeAction::All | ChangeAction::SetItemData); - ADD_POINTER(tableOfEnumRoles) - << 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); + ADD_ALL(listOfNamedRoles, 1, ChangeAction::ChangeRows | ChangeAction::SetData | ChangeAction::SetItemData); + + ADD_ALL(tableOfEnumRoles, 1, ChangeAction::All | ChangeAction::SetItemData); + + ADD_ALL(tableOfIntRoles, 1, ChangeAction::All | ChangeAction::SetItemData); + + ADD_ALL(stdTableOfIntRoles, 1, ChangeAction::All | ChangeAction::SetItemData); #undef ADD_COPY #undef ADD_POINTER #undef ADD_HELPER +#undef ADD_ALL QTest::addRow("Moved table") << Factory([]{ QList> movedTable = { @@ -727,15 +701,19 @@ void tst_QGenericItemModel::insertRows() QCOMPARE(model->rowCount() == expectedRowCount + 1, changeActions.testFlag(ChangeAction::InsertRows)); - auto ignoreFailureFromAssociativeContainers = []{ - QEXPECT_FAIL("listOfNamedRolesPointer", "QVariantMap is empty by design", Continue); - QEXPECT_FAIL("listOfNamedRolesCopy", "QVariantMap is empty by design", Continue); - QEXPECT_FAIL("tableOfEnumRolesPointer", "QVariantMap is empty by design", Continue); - QEXPECT_FAIL("tableOfEnumRolesCopy", "QVariantMap is empty by design", Continue); - QEXPECT_FAIL("tableOfIntRolesPointer", "QVariantMap is empty by design", Continue); - QEXPECT_FAIL("tableOfIntRolesCopy", "QVariantMap is empty by design", Continue); - QEXPECT_FAIL("stdTableOfIntRolesPointer", "std::map is empty by design", Continue); - QEXPECT_FAIL("stdTableOfIntRolesCopy", "std::map is empty by design", Continue); + auto ignoreFailureFromAssociativeContainers = [] { + for (auto suffix : { "Pointer", "Copy", "Ref" }) { + auto addCase = [suffix](const std::string& testName, + const std::string& containerName) { + QEXPECT_FAIL((testName + suffix).c_str(), + (containerName + " is empty by design").c_str(), + 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);