Hi Milian,
Thanks for looking at my patches. Since the branch, as you noticed, is quite
contaminated with unrelated changes, merging in and out stuff, I'll push these
changes to a cleaned branch, and will address the issues you pointed out. I'll
post a new RR then.
On Sunday, November 16, 2014 14:13:39 Milian Wolff wrote:
> On Thursday 06 November 2014 03:44:58 Sebastian Kügler wrote:
> > Hi all, especially Alex and David,
> >
> > tl;dr:
> > I've done a proof-of-concept implementation of a metadata index for
> > KPluginTrader::query(), the main entry point when it comes to finding
> > binary plugins. This index considerably speeds up all current use cases,
> > but comes at the cost of having to maintain the index. Code is in
> > kservice[sebas/kpluginindex], speeds up plugin quering a few times.
>
> <snip>
>
> Hey Sebas,
>
> I just took a look at your code. What comes to my mind you'll find below.
>
> a) you added a "return;" in the middle of a pluginlocatortest - why? no
> comment either. If you expect failures, use QEXPECT_FAIL. I've added this
> also to PluginTest::findPackageStructure now.
>
> b) adding code in a merge commit is a very bad idea. Never do that. (see
> 3c9f33251708c6d935ed35fbfb6130c4389e69a7). One reason is that merge commits
> are "special" and often omitted when looking at code. See e.g. "git log -u"
> which won't show the code you added in the merge...
>
> c) The code seems to contain tons of debug code, commented out code etc.
> pp.. Will you please clean that up before you merge this in? I.e. squash it
> locally with the cleanup commits to not uglify the git history.
>
> d) Overall I find it extremely hard to review the code since there is so
> much unrelated stuff going on. Now that Alexander's work is in master (it
> is, no?) could you maybe cleanup your branch and do a force-push rebased on
> top of current master? If you do that together with c) I could actually see
> whats supposed to be going on without being confused by tons of new code
> that's only there for debug purposes.
>
> e) // Less than two plugin means it's not worth indexing
>
> I'd argue this code path can be removed. In a normal session there will
> always be more than two plugins.
>
> f) I urge you (and everybody else) to rethink some code structures around
> conditionals. Most of the time one can greatly simplify functions (imo) by
> the following (inspired by KPluginIndexer::removeIndex):
>
> bool ok = true;
> if (foo) {
> if (bar) {
> if (!asdf) {
> ok = false;
> qWarning()
> } else {
> // success
> }
> } else {
> qWarning();
> ok = false;
> }
> }
> return ok;
>
> rewrite this as:
>
> if (!foo) {
> return true;
> }
> if (!bar) {
> return false;
> }
> if (!asdf) {
> return false;
> }
> // success!
>
> g) I still don't know how to benchmark your code.
>
> Bye
--
sebas
http://www.kde.org | http://vizZzion.org | GPG Key ID: 9119 0EF9
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
