This one is serious because it impacts actual runtime, not just tests.
Details
- Reviewers
dvratil - Commits
- R165:548c54a1b25c: Fix leaking of requests in ItemRetriever::exec()
ctest -R itemretrievertest in an ASAN build
Diff Detail
- Repository
- R165 Akonadi
- Branch
- itemretriever (branched from Applications/19.04)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 11742 Build 11760: arc lint + arc unit
Let's fix this properly and get rid of the new/delete completely. Could you wrap the value in std::shared_ptr/QSharedPointer?
I don't really like shared_ptr for things like this. To me shared_ptr means "ownership is unclear" (when misused) or "ownership is shared between multiple objects, we need refcounting so that the last user deletes it".
This isn't the case here. ItemRetriever creates the requests, ItemRetrievalManager processes them (but never owns them), and then we need to delete them after use.
A refcount is unnecessary (i.e. overkill) and makes it harder to find out the ownership situation by reading the code.
The owner is clear and unique: it's the requests list. Hence the delete after the removeOne.
Not readyItems, but requests should then be QVector<std::unique_ptr<ItemRetrievalRequest>> as a clear owner of the object.
Port to std::vector<std::unique_ptr<ItemRetrievalRequest>>
The code isn't exactly simpler, in this particular case...
src/server/storage/itemretriever.cpp | ||
---|---|---|
284 | Good suggestion, thanks. | |
332–335 | A for loop better than find_if? What makes you say that? It's general consensus that using an algorithm is always better than writing a for loop. |
src/server/storage/itemretriever.cpp | ||
---|---|---|
332–335 | I made a wrong assumption, i mean well readable for (auto it = requests.begin(); it != requests.end(); ++it) { if (it->get() == finishedRequest) { |