Remove ::Ptr usage from SqliteFormat #56
Conversation
|
Needs rebase and some adjustments, doesn't compile now on top of the search model changes. |
|
I've corrected the code introduced by the search PR. Tests are passing as far as I can tell. Thank you for the notice. |
src/sqlitestorage.cpp
Outdated
| @@ -656,7 +656,7 @@ int SqliteStorage::Private::loadIncidencesBySeries(sqlite3_stmt *stmt1, QStringL | |||
|
|
|||
| while ((incidence = mFormat->selectComponents(stmt1, notebookUid)) | |||
| && (limit <= 0 || count < limit)) { | |||
| if (addIncidence(incidence, notebookUid)) { | |||
| if (addIncidence(Incidence::Ptr(incidence), notebookUid)) { | |||
There was a problem hiding this comment.
This looks like potentially leaking memory. The selectComponents() returns an instance owned now by the caller, but then there are other filters above which can make this ctor skipped.
Perhaps safer if either the "incidence" instance is kept as Ptr or switched into some other similar type e.g. QScopedPointer, and the while loop assignment constructs a proper wrapped instance, or then we don't touch the return types at all if it should be Ptr always.
Maybe just better if we don't return instances owned by the caller?
There was a problem hiding this comment.
Ah, that's a good remark. Returning a pointer like that and not taking ownership immediately can be very error prone, as you demonstrated.
I'm withdrawing the third commit completely introducing this. Read routines will continue to return a QSharedPointer for the moment, up to when a real case for change appears.
|
Indeed not sure how much this in practice helps, but cleaning up the side-effects to the incidences is nice and I have no objections either. So let's just have. |
@pvuorela, this is a proposition not to use ::Ptr internally in SqliteFormat. This is not really mandatory at the moment, nor simplifying much the code, but it helps to disconnect the SQLite backend from a calendar (and also does not pin the incidence ownership to the current thread). Now, SqliteFormat is just taking plain Incidence references to write to the database, or returning new objects on read that the caller is responsible to take ownership of.
It allows to catch some minor issues like changing some internal values of incidence on write, while it should be const.