There were 3 QMaps with identical keys used for storing corresponding
view, proxy model and filter. In this version only one map is used - this
reduces the amount of map lookups and potential mistakes during code
changes.
Details
- Reviewers
kfunk - Group Reviewers
KDevelop - Commits
- R32:252482d18816: Eliminate duplicate QMaps in OutputWidget
This patch isn't supposed to change any behavior in OutputWidget, so we
need to assure that OutputWidget works as usually. For example:
create a few outputs;
enter the filters for some of the outputs;
choose random output;
close all other outputs;
check if the output is shown, filter corresponds to output, etc;
repeat previous actions a few times.
Diff Detail
- Repository
- R32 KDevelop
- Branch
- refactor_outputwidget (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 2001 Build 2019: arc lint + arc unit
plugins/standardoutputview/outputwidget.cpp | ||
---|---|---|
278–290 | This can be simplified in one single lookup: auto view = m_views.take(id); view.destroy(); | |
292–293 | Ditto. | |
704–711 | delete is a no-op on null pointers, so you can simply do: delete foo; foo = nullptr; Of course, assuming you do not need to use the pointer just before its deletion. | |
plugins/standardoutputview/outputwidget.h | ||
110 | I'd personally name it findFilteredView(), and make it const, and return a const_iterator. At least from a quick glance, it seems like the iterator returned is always not changed/removed. | |
112 | Unless you need the values sorted by the key (which is QMap does), I suggest to switch also to QHash. |
Multiple improvements corresponding to review comments
Use QHash instead of QMap
Rename getFilteredView() to findFilteredView()
Remove filtered view in one lookup
Don't compare pointers to nullptr before delete in FilteredView::destroy()
plugins/standardoutputview/outputwidget.h | ||
---|---|---|
110 | I was thinking about returning the const_iterator, but the returned value is changed at least in OutputWidget::outputFilter() (lines 658 and 663). |
plugins/standardoutputview/outputwidget.h | ||
---|---|---|
108 | If I'm not mistaken you could omit the destroy() function (and probably a few other deletes) completely by making the two pointers in the struct a QSharedPointer. I think this would make the code a little bit more clean. What do you think? |
plugins/standardoutputview/outputwidget.h | ||
---|---|---|
108 | Well, frankly speaking, I'm not sure that this will make the code cleaner. A lot of lines in the code use the raw pointer to QTreeView objects, so I'll need to take out the raw pointer from QSharedPointer, which is not quite clean. |
Thanks for your change even if you were not happy about it.
So what do you think? To me that is easier to follow, and gives very few chances for accidental leaks. One should use smart pointers more often these days.
Note: In general this patch looks good to me.
plugins/standardoutputview/outputwidget.cpp | ||
---|---|---|
278–289 | I think this line is redundant. You're doing m_views.remove(id); outside the if-else stmt anyway(?) |
plugins/standardoutputview/outputwidget.cpp | ||
---|---|---|
278–289 | Yes, thanks, it's redundant, it leaked into the code because I was thinking about the issue, described in the TODO comment below. I'll remove it now. |
You're welcome :) And no, I'm quite happy with the way it is now too.
Now, talking about memory leaks - I tried to check this after your previous comment and, as far I understood, we'll get no memory leaks here even without destroy() with raw pointers, because both proxy model and view have parents and will be deleted by them. But yes, if somebody changes the code and removes the parent unintentionally, that will lead to memory leak, so you're right that the chances of such scenario are reduced now.
Hi @aaronpuchert, thank you for comment.
I was in doubt between these two variants while writing the code and stopped on current one for two reasons:
- Looking at QT source code, reset() is ultimately reduced to QSharedPointer copy(t); swap(copy); anyways. And QSharedPointer has move constructor, so it'll work in this case (right?).
- Current variant better expresses my intentions (that's subjective).
But if you still think that we should use reset() I'd change it in a moment. What do you think?
P.S. Sorry for that nit-picking. I'm still just learning, so I have a time and desire to make these long investigations about whether it's better to use reset or assignment, shared pointers or raw pointers, etc. I think that'll pass after a few more patches XD
These were just suggestions. Using reset tends to be a bit shorter because you don't have to write the type name again.
There should be no functional change between the two variants, at least I see none.
Hi @kfunk, you've told that the patch looks good in general except for the redundant m_views.remove(id); call - I've deleted it. So what about the patch now? Should anything else be changed or can it be accepted now?
plugins/standardoutputview/outputwidget.cpp | ||
---|---|---|
530 | It's not correct, 3 line above you get a raw managed pointer, here you make a distinct managed pointer with same source. It'll result in double deletion when goes out of scope. |
plugins/standardoutputview/outputwidget.cpp | ||
---|---|---|
260 | KDevelop coding style is to have the * with the type, so please keep that (QTreeView*) | |
530 | As @anthonyfieroni said, there is a bug here: listview = m_views.begin().value().view.data(); takes a war copy of the pointer otherwise controlled by a QSharedPointer value. Please fix this by making listview a QSharedPointer itself, so in case an existing pointer is taking, there will be only one set of QSharedPointer around that. | |
plugins/standardoutputview/outputwidget.h | ||
110 | Please remove the "get" prefix, name the method simply filteredView(QAbstractItemView *view); |
@anthonyfieroni, @kossebau, yes, that's definitely a bug. I didn't notice it, sorry for that. Should I fix it as a new commit to this patch or create a new patch?
plugins/standardoutputview/outputwidget.cpp | ||
---|---|---|
260 | Got it. I understood it vice versa after reading the wiki. Will do it right way from now on. |
@anthonyfieroni, got it, thank you for answer. I'll try to submit the patch with fix today.
@kossebau , @anthonyfieroni I've sumbitted the patch which should fix the bug: D15241. Hope that it's correct this time.