Hi, @Igor, @Volker: Thanks for your feedback!
QPersistentModelIndex is not a proper solution here, for a practical reason: are you going to use QPMI *everywhere*? We experience crashes not only in our code but inside Qt's own code, QSortFilterProxyModel, QIdentityProxyModel, etc. which rely on internalPointer for their own bookkeeping. No amount of QPMI in application code fixes that. Using QPMI everywhere is also quite hacky; it's like having a QPersistentString that I must use to check whether my QString is still valid... On "QModelIndex should be thought of like an STL iterator": the analogy is forced, because with an STL container *I* have full control. The iterator is only invalidated when *my* code changes the container. With QAIM, framework code invalidates indices behind my back: any Q*ModelView or delegate may call QCoreApplication::processEvents(), and Qt's own signals carry QModelIndex by value across queued connections, so an index can already be stale at the moment it is delivered. A single posted dataChanged (or any structural signal) is enough to make internalPointer point to something that no longer exists. This is the daily reality of highly dynamic models, the current contract is not just hard to follow, Qt itself breaks it on my behalf. > And the model knows when a model index might have been invalidated, and > would need to somehow update the payload in constructed QModelIndex > instances - what should QModelIndex::internalData() return if the index > has been removed from the model? It returns the QVariant. :) That's the point: the payload is a *value*, not a reference into model state, so there is nothing for the model to update. internalData() returns whatever was stored when the index was created, and accessing it is always safe, removed from the model or not. What that value *means* is the model's business: store a shared_ptr and the data stays alive as long as any index references it; store a weak_ptr and lock() failing tells you the item is gone, so you bail out gracefully. Compare with internalPointer(), where the model equally cannot update anything, but the result is a dangling pointer and UB. > So what do we win? Defined behavior! :) > a view or other generic client code cannot generally assume anything > about what internalData() might return Of course, the problem is on the model side where we don't know if internalPointer is still valid or not. E.g. QSortFilterProxyModel *is* a model, and it is precisely where these crashes happen today. Qt's own proxies would be the first beneficiaries ;-) . > This can already be done by anyone right now, just adding a Qt::UserRole > to the model to return a key-for-index, and implementing a persistent > mapping from key-to-item/index in their model. We do exactly this today (an in-house bimap between ids and items), and yes, it works, a lookup miss is how we detect a stale index. But look at what it costs: a parallel data structure that must be maintained in lockstep with the model, CRUD on the map for every item created or destroyed, and a lookup on every single access, on the hottest path in model/view. And every author of a dynamic model has to hand-roll this same boilerplate; it is reference counting, reimplemented badly, once per model. A shared_ptr in the payload is the same validity guarantee for an O(1) refcount and zero bookkeeping. And it doesn't help Qt's own proxies either way: QSortFilterProxyModel cannot use my application-level bimap, so its internalPointer crashes remain no matter how good my map is. On cost: rather than debate this in the abstract, I went ahead and implemented it for Qt 7: https://codereview.qt-project.org/c/qt/qtbase/+/749550. QModelIndex does lose trivial copyability, but an empty payload costs next to nothing at copy time, so only models that opt in actually pay. I'll attach benchmarks for the index-heavy paths (views, QSFPM) to the review, so we can settle the cost question on numbers rather than assumptions. Cheers, BogDan. -- Development mailing list [email protected] https://lists.qt-project.org/listinfo/development
