[RFC] [kservice] KPluginMetadata indexing

[Milian Wolff]

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
-- 
Milian Wolff
mail at milianw.de
http://milianw.de