> On Nov. 17, 2014, 9:45 nachm., Thomas Lübking wrote:
> > attica-kde/kdeplugin/kdeplatformdependent.cpp, line 56
> > <https://git.reviewboard.kde.org/r/121161/diff/2/?file=328928#file328928line56>
> >
> > is
> >
> > KdePlatformDependent::~KdePlatformDependent()
> > {
> > if (QCoreApplication::closingDown())
> > m_accessManager->setParent(nullptr);
> > }
> >
> > sufficient?
>
> Albert Astals Cid wrote:
> Isn't that a more complex way of doing the same?
>
> Thomas Lübking wrote:
> Depends on whether KdePlatformDependent is ever deleted in a running
> application (where the leak would really matter)
> If not, then yes: superfluous complication.
>
> Jeremy Whiting wrote:
> Did I hear a "Ship it!" in there somewhere?
>
> Thomas Lübking wrote:
> Leaving aside that don't maintain the attica plugin, you have a "+1" from
> here, IF
> a) the conditional reparent on destruction is not sufficient (doesn't
> work)
> OR
> b) KdePlatformDependent object(s) are not supposed to be deleted during
> the runtime of the application
>
> Ie. if the leak is inevitable or only theoretical.
>
> Jeremy Whiting wrote:
> Yep, I'm one of the attica and knewstuff maintainers and got the plugin
> to load recently, then hit this crash on close, so trying to fix it. The code
> in the plugin is only built with the plugin, so shouldn't be used besides as
> a plugin.
>
> The conditional reparent probably works too, but as Albert said it's just
> a more complicated way of doing the same thing.
> The code in the plugin is only built with the plugin, so shouldn't be used
> besides as a plugin.
That's not what I asked.
This:
int main(int, char**) {
KdePlatformDependent dep; // lives as long as the process
}
does not actually "leak".
This:
int main(int, char**) {
for (int i = 0; i < 100; ++i)
KdePlatformDependent dep; // lives for the iteration only
}
does.
If the change causes an *actual* leak and that's avoidable, I'd suggest to
avoid it.
If not, than it really doesn't matter.
- Thomas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121161/#review70542
-----------------------------------------------------------
On Nov. 17, 2014, 9:11 nachm., Jeremy Whiting wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121161/
> -----------------------------------------------------------
>
> (Updated Nov. 17, 2014, 9:11 nachm.)
>
>
> Review request for kdelibs and Plasma.
>
>
> Repository: plasma-desktop
>
>
> Description
> -------
>
> Plugin unload order was making the attica_kde plugin crash on close,
> this works around it by leaking one AccessManager.
>
>
> Diffs
> -----
>
> attica-kde/kdeplugin/kdeplatformdependent.cpp
> 489c03b18b7bb940007ab51cb197105fbc25de9f
>
> Diff: https://git.reviewboard.kde.org/r/121161/diff/
>
>
> Testing
> -------
>
> knewstuff tests no longer crash on close.
>
>
> Thanks,
>
> Jeremy Whiting
>
>
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
