Eliminate duplicate QMaps in OutputWidget
ClosedPublic

Authored by vkorneev on Aug 19 2018, 6:37 PM.

Details

Summary

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.

Test Plan

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
vkorneev created this revision.Aug 19 2018, 6:37 PM
Restricted Application added a project: KDevelop. · View Herald TranscriptAug 19 2018, 6:37 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
vkorneev requested review of this revision.Aug 19 2018, 6:37 PM
pino added a subscriber: pino.Aug 19 2018, 8:19 PM
pino added inline comments.
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.

vkorneev updated this revision to Diff 40016.Aug 19 2018, 9:07 PM

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()

vkorneev added inline comments.Aug 19 2018, 9:10 PM
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).

kfunk added a subscriber: kfunk.Aug 23 2018, 7:00 AM
kfunk added inline comments.
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?

vkorneev added inline comments.Aug 23 2018, 5:13 PM
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.
I'll rewrite it now, of course, so we'll be able to compare both versions and decide which one is better.

vkorneev updated this revision to Diff 40316.Aug 23 2018, 5:57 PM

Use QSharedPointers instead of raw pointers in FilteredView struct

kfunk added a comment.Aug 23 2018, 7:11 PM

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.

kfunk added a comment.Aug 23 2018, 7:14 PM

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(?)

vkorneev added inline comments.Aug 23 2018, 7:27 PM
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.

vkorneev updated this revision to Diff 40318.Aug 23 2018, 7:29 PM

Remove redundant m_views.remove()

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.

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.

aaronpuchert added inline comments.
plugins/standardoutputview/outputwidget.cpp
286

You can also write fview.proxyModel.reset().

530

And here you can write m_views[id].view.reset(listview).

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

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?

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?

anthonyfieroni added inline comments.
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.

kfunk accepted this revision.Sep 2 2018, 7:16 PM

LGTM

This revision is now accepted and ready to land.Sep 2 2018, 7:16 PM
This revision was automatically updated to reflect the committed changes.
kossebau added inline comments.
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.
And
`
m_views[id].view = QSharedPointer<QTreeView>(listview);
`
then creates a new QSharedPointer around that raw pointer. As a result we have two sets of QSharedPointer for the very same pointer, which will result in double deletion once both sets go out of existance.

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?

When one patch is merged always make a new one, force push is disallow in KDE.

vkorneev added inline comments.Sep 3 2018, 7:19 AM
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.