----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127795/#review95047 -----------------------------------------------------------
Note first off that I'm not a Plasma dev, so some of these comments are questions more than they are statements. I do see several issues though: - It seems to me that there's a lot of changes in here which aren't actually necessary to fix the bug you're talking about. - The bug description is extremely sparse. There is presumably a memory leak... how did you notice it? How did you isolate it? Did you find a particular bug and then fix that, or did you make changes here and there until you couldn't reproduce the bug anymore? We prefer bug fixes where the bug *and* the solution are well-understood. ;) But we rely on good descriptions of what the bug is and how it's now fixed (instead of 'see title'). - I believe the Plasma developers are on a separate group; you should edit the review request to add the 'plasma' group to ensure this is reviewed by developers with actual Plasma experience. With all that said I do believe you've found at least one leak and closed it off, but it's hard to tell that from the patch and minimal description that's been submitted. autotests/pluginloadertest.cpp (line 84) <https://git.reviewboard.kde.org/r/127795/#comment64507> `engine != 0` can simply be `engine` in this bool context. Plus you should really prefer `Q_NULLPTR` to `0` in modern code anyways even if you wanted to check directly. src/plasma/datacontainer.cpp (line 162) <https://git.reviewboard.kde.org/r/127795/#comment64505> Although I've made a lot of questions regarding usage of Qt::UniqueConnection elsewhere, this one may be necessary since I don't see any other checks for whether this signal-slot connection already exists. src/plasma/datacontainer.cpp (line 170) <https://git.reviewboard.kde.org/r/127795/#comment64506> This and the next 3 Qt::UniqueConnection flags appear to be unneeded: by this point in the code path there have already been checks for these signal-slot connections and what appear to be correct calls to QObject::disconnect(). src/plasma/dataengine.cpp (line 269) <https://git.reviewboard.kde.org/r/127795/#comment64504> Although for this instance of Qt::UniqueConnection, I can't prove it impossible that duplicates would occur, this code path does have a guard to avoid duplicate sources being created for this data engine. Are we sure these Qt::UniqueConnection flags are necessary, or is this experimentation? On that note, it might be a good idea to combine this code path with common portions DataEnginePrivate::source(QString,bool) in order to ensure there's one (and only one) correct way to add a source to a data engine. That would help for later code audits when trying to verify where (and when) QObject connections are created. src/plasma/dataengine.cpp (line 416) <https://git.reviewboard.kde.org/r/127795/#comment64500> This should be 1 instead of 0 given the changes you've made elsewhere (unless the existing value is a bug...). But, the entire set of changes to reference count handling appear to be unnecessary. Why the change? src/plasma/dataengine.cpp (line 521) <https://git.reviewboard.kde.org/r/127795/#comment64501> The DataContainer `s` has only just been created by this point in the code, so `Qt::UniqueConnection` should be meaningless. Likewise for the change 2 lines down. src/plasma/dataengine.cpp (line 606) <https://git.reviewboard.kde.org/r/127795/#comment64503> By definition the source here has been created for the first time in this code path. So Qt::UniqueConnection should be useless. src/plasma/private/dataenginemanager.cpp <https://git.reviewboard.kde.org/r/127795/#comment64502> Is it safe to remove this, given that even the change to DataEngineManager::loadEngine to ->ref all returned engines still doesn't ->ref the null engine? - Michael Pyne On April 30, 2016, 3:18 p.m., Anthony Fieroni wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127795/ > ----------------------------------------------------------- > > (Updated April 30, 2016, 3:18 p.m.) > > > Review request for KDE Frameworks and Marco Martin. > > > Repository: plasma-framework > > > Description > ------- > > ^^ > > > Diffs > ----- > > autotests/pluginloadertest.h 61fc963 > autotests/pluginloadertest.cpp 4f96780 > src/plasma/datacontainer.cpp ee067db > src/plasma/dataengine.cpp f942926 > src/plasma/private/dataenginemanager.cpp 08e42fb > > Diff: https://git.reviewboard.kde.org/r/127795/diff/ > > > Testing > ------- > > Test pass. > > > Thanks, > > Anthony Fieroni > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
