diff --git a/src/corelib/itemmodels/qgenericitemmodel.h b/src/corelib/itemmodels/qgenericitemmodel.h index 0408fb39dca..90683049152 100644 --- a/src/corelib/itemmodels/qgenericitemmodel.h +++ b/src/corelib/itemmodels/qgenericitemmodel.h @@ -874,8 +874,7 @@ public: { // erase invalidates iterators const auto begin = QGenericItemModelDetails::pos(children, row); const auto end = std::next(begin, count); - if constexpr (rows_are_raw_pointers) - that().deleteRemovedRows(begin, end); + that().deleteRemovedRows(begin, end); children->erase(begin, end); } // fix the parent in all children of the modified row, as the @@ -939,16 +938,18 @@ public: protected: ~QGenericItemModelImpl() { - if constexpr (rows_are_raw_pointers && !std::is_pointer_v - && !QGenericItemModelDetails::is_any_of()) { - // If data with rows as pointers was moved in, then we own it and - // have to delete those rows. - using ref = decltype(std::forward(std::declval())); - if constexpr (std::is_rvalue_reference_v) - that().destroyOwnedModel(*m_data.model()); + // We delete row objects if we are not operating on a reference or pointer + // to a range, as in that case, the owner of the referenced/pointed to + // range also owns the row entries. + // ### Problem: if we get a copy of a range (no matter if shared or not), + // then adding rows will create row objects in the model's copy, and the + // client can never delete those. But copied rows will be the same pointer, + // which we must not delete (as we didn't create them). + if constexpr (protocol_traits::has_deleteRow && !std::is_pointer_v + && !QGenericItemModelDetails::is_any_of()) { + const auto begin = QGenericItemModelDetails::begin(*m_data.model()); + const auto end = QGenericItemModelDetails::end(*m_data.model()); + that().deleteRemovedRows(begin, end); } } @@ -958,10 +959,8 @@ protected: // If we operate on dynamic columns and cannot resize a newly // constructed row, then we cannot insert. return false; - } else if constexpr (!one_dimensional_range && !rows_are_raw_pointers - && !protocol_traits::initializes_rows) { - // We also cannot insert if the row we have to create is not - // default-initialized by the protocol + } else if constexpr (!protocol_traits::has_newRow) { + // We also cannot insert if we cannot create a new row element return false; } else if constexpr (!range_features::has_insert_range && !std::is_copy_constructible_v) { @@ -1414,19 +1413,13 @@ protected: return empty_row; } - template - void destroyOwnedModel(R &&range) - { - const auto begin = QGenericItemModelDetails::begin(range); - const auto end = QGenericItemModelDetails::end(range); - deleteRemovedRows(begin, end); - } - template void deleteRemovedRows(It &&begin, Sentinel &&end) { - for (auto it = begin; it != end; ++it) - this->protocol().deleteRow(*it); + if constexpr (tree_traits::has_deleteRow) { + for (auto it = begin; it != end; ++it) + this->protocol().deleteRow(*it); + } } void resetParentInChildren(range_type *children) @@ -1627,19 +1620,13 @@ protected: return empty_row; } - template - void destroyOwnedModel(R &&range) - { - const auto begin = QGenericItemModelDetails::begin(range); - const auto end = QGenericItemModelDetails::end(range); - for (auto it = begin; it != end; ++it) - delete *it; - } - template - void deleteRemovedRows(It &&, Sentinel &&) + void deleteRemovedRows(It &&begin, Sentinel &&end) { - // nothing to do for tables and lists as we never create rows either + if constexpr (Base::protocol_traits::has_deleteRow) { + for (auto it = begin; it != end; ++it) + this->protocol().deleteRow(*it); + } } decltype(auto) rowDataImpl(const QModelIndex &index) const diff --git a/src/corelib/itemmodels/qgenericitemmodel_impl.h b/src/corelib/itemmodels/qgenericitemmodel_impl.h index bbadb5ffdcc..1970e432dbd 100644 --- a/src/corelib/itemmodels/qgenericitemmodel_impl.h +++ b/src/corelib/itemmodels/qgenericitemmodel_impl.h @@ -72,8 +72,8 @@ namespace QGenericItemModelDetails QExplicitlySharedDataPointer, QSharedDataPointer>; template - using is_owning_or_raw_pointer = std::disjunction, is_any_unique_ptr, std::is_pointer>; - + using is_owning_or_raw_pointer = std::disjunction, is_any_unique_ptr, + std::is_pointer>; template static auto pointerTo(T&& t) { @@ -354,10 +354,9 @@ namespace QGenericItemModelDetails using row_type = typename range_traits>::value_type; template >, bool> = true, - std::enable_if_t::value, bool> = true> - auto newRow() -> decltype(R(new wrapped_t)) - { + std::enable_if_t>, + is_owning_or_raw_pointer>, bool> = true> + auto newRow() -> decltype(R(new wrapped_t)) { if constexpr (is_any_of()) return std::make_shared>(); else @@ -365,13 +364,12 @@ namespace QGenericItemModelDetails } template >, bool> = true, - std::enable_if_t>, bool> = true> + std::enable_if_t::value, bool> = true> auto newRow() -> decltype(R{}) { return R{}; } - template , bool> = true> - auto deleteRow(R row) -> decltype(delete(row)) { delete row; } + template >, bool> = true> + auto deleteRow(R&& row) -> decltype(delete row) { delete row; } }; template >::value_type> @@ -485,7 +483,6 @@ namespace QGenericItemModelDetails static constexpr bool has_setParentRow = protocol_setParentRow(); static constexpr bool has_mutable_childRows = protocol_mutable_childRows(); - static constexpr bool initializes_rows = has_newRow && !is_any_of(); static constexpr bool is_default = is_any_of(); }; diff --git a/tests/auto/corelib/itemmodels/qgenericitemmodel/tst_qgenericitemmodel.cpp b/tests/auto/corelib/itemmodels/qgenericitemmodel/tst_qgenericitemmodel.cpp index 1c9a959b4c9..012fc15e7a2 100644 --- a/tests/auto/corelib/itemmodels/qgenericitemmodel/tst_qgenericitemmodel.cpp +++ b/tests/auto/corelib/itemmodels/qgenericitemmodel/tst_qgenericitemmodel.cpp @@ -474,7 +474,7 @@ private: Object row2; Object row3; std::list listOfObjects = { - &row1, &row2, &row3 + new Object, new Object, new Object }; MetaObjectTuple mot1; @@ -896,10 +896,12 @@ void tst_QGenericItemModel::ownership() std::vector objects { object }; - { // model does not take ownership + { // model takes ownership of its own copy of the vector (!) QGenericItemModel modelOnCopy(objects); } - QVERIFY(guard); + QVERIFY(!guard); + objects[0] = new Object; + guard = objects[0]; { // model does not take ownership QGenericItemModel modelOnPointer(&objects); } @@ -911,6 +913,7 @@ void tst_QGenericItemModel::ownership() { // model does take ownership QGenericItemModel movedIntoModel(std::move(objects)); + QCOMPARE(movedIntoModel.columnCount(), 2); } QVERIFY(!guard); }