D9100: KUriFilter: simplify data structures, fix memory leak
dfaure added inline comments. INLINE COMMENTS > emateli wrote in kurifilter.cpp:597 > Perhaps this can be merged with the if statement above. It can, but I would certainly find it much less readable. The heart of the method is to call filterUri. The first if() is "should I call it?" The nested if() is "did it work?" A single if statement mixing both, would certainly make this harder to read. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9100 To: dfaure, apol, davidedmundson, arichardson, bshah Cc: emateli, #frameworks
D9100: KUriFilter: simplify data structures, fix memory leak
emateli added inline comments.
INLINE COMMENTS
> kurifilter.cpp:597
> +if (filters.isEmpty() || filters.contains(plugin->objectName())) {
> +if (plugin->filterUri(data)) {
> filtered = true;
Perhaps this can be merged with the if statement above.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D9100
To: dfaure, apol, davidedmundson, arichardson, bshah
Cc: emateli, #frameworks
D9100: KUriFilter: simplify data structures, fix memory leak
This revision was automatically updated to reflect the committed changes. Closed by commit R241:73fd5f771cc4: KUriFilter: simplify data structures, fix memory leak (authored by dfaure). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D9100?vs=23254=23278#toc REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9100?vs=23254=23278 REVISION DETAIL https://phabricator.kde.org/D9100 AFFECTED FILES autotests/kurifiltertest.cpp autotests/kurifiltertest.h src/widgets/kurifilter.cpp src/widgets/kurifilter.h To: dfaure, apol, davidedmundson, arichardson, bshah Cc: #frameworks
D9100: KUriFilter: simplify data structures, fix memory leak
dfaure planned changes to this revision.
dfaure added inline comments.
INLINE COMMENTS
> kurifilter.cpp:665
> +QStringList res;
> +res.reserve(d->pluginList.size());
> +std::transform(d->pluginList.constBegin(), d->pluginList.constEnd(),
> res.begin(), [](const KUriFilterPlugin *plugin) { return
> plugin->objectName(); });
Interesting, to review my own commit from a month ago, after I forgot its
details...
Dude, this code crashes, reserve+transform needs back_inserter, otherwise use
resize!
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D9100
To: dfaure, apol, davidedmundson, arichardson, bshah
Cc: #frameworks
D9100: KUriFilter: simplify data structures, fix memory leak
apol accepted this revision. apol added a comment. This revision is now accepted and ready to land. It's interesting how the code ends up simpler. :) REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D9100 To: dfaure, apol, davidedmundson, arichardson, bshah Cc: #frameworks
D9100: KUriFilter: simplify data structures, fix memory leak
dfaure created this revision. dfaure added reviewers: apol, davidedmundson, arichardson, bshah. Restricted Application added a project: Frameworks. REVISION SUMMARY This code had a QHash and a separate ordered list of names in order to use plugins in the right order. It's much simpler to just have a vector of plugins instead. Also, this fixes a memory leak when multiple plugins are found with the same name (e.g. one from /usr and one from my custom prefix, although the right fix for this particular issue is to load only one of them). TEST PLAN kurifilter tests still pass REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D9100 AFFECTED FILES src/widgets/kurifilter.cpp src/widgets/kurifilter.h To: dfaure, apol, davidedmundson, arichardson, bshah Cc: #frameworks
