Subject: Fixes memory leak in KUrlComboBox::setUrl
Details:
If setUrl is called multiple times it first removes the previous item
but fails to delete the item before removing it causing a memory leak.
Details
Details
- Reviewers
dfaure - Commits
- R241:f71092c8c999: Fixes memory leak in KUrlComboBox::setUrl
Unit test
Diff Detail
Diff Detail
- Repository
- R241 KIO
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 1664 Build 1682: arc lint + arc unit
| src/widgets/kurlcombobox.cpp | ||
|---|---|---|
| 356–357 | Sure has :) I was thinking if a nicer solution would be to use a smart pointer in the itemList instead? That way we wouldn't need to go hunting for this and risk introducing leaks again? Do we have any favorite smart pointer for this? Or would a std::unique_ptr be fine? | |
Comment Actions
Why shared? I don't see any ownership sharing happening. The list owns the pointers. I'm always wary of shared_ptr because it's often overused as "we don't really know who's responsible for deleting this, so let's just refcount it". But there's no refcounting needed here. The ownership is very clear here.
Comment Actions
My first approach was to use unique_ptr, but then I realized that pointers are shared between the defaultList and the itemMapper (see for example line 164) and also between the itemList and the itemMapper (see line 278, 291 and others), but I can't see any sharing between the defaultList and itemList. I thought of using a std::weak_ptr in the itemMapper, but I am not sure that entries are always removed from itemMapper when the are removed from the defaultList or itemMapper, also if the itemMapper should be able to hold objects from both lists I think it is more sane to use a shared_ptr.
Let me know what you think
Comment Actions
The ownership was clear in the original code: itemList and defaultList own the items, the rest (like itemMapper) doesn't.
So unique_ptrs in itemList and defaultList, and raw pointers elsewhere, would be perfectly fine and much clearer.
Don't fall into the "no raw pointers" trap. The correct recommendation is "no raw pointers that own the object".
Comment Actions
Use remove_if for removing entries from itemList and defaultList.
Added unit test of KUrlComboBox::removeUrl.
| src/widgets/kurlcombobox.cpp | ||
|---|---|---|
| 359 | In general this class is not covered very well by unit test, I have added a test case that covers most of the removeUrl functionality, but I think I will do a different patch with more unit test. | |
Comment Actions
No, not yet - do you think I should apply for one? I would really like to continue posting patches :)
Comment Actions
Yes, I think you should apply for one. You can quote my name when the form asks who recommended you.