[Nepomuk] Review Request 109991: Regexp cache optimization in Nepomuk fileindexer.

[Lukasz Olender]

> On April 17, 2013, 1:50 a.m., Simeon Bird wrote:
> > common/regexpcache.cpp, line 97
> > <http://git.reviewboard.kde.org/r/109991/diff/1/?file=138503#file138503line97>
> >
> >     Why not just QString()?

I tried to create more self-describing code, which tells that regexp will be empty, but I'm not really sure about it - should I put just QString()?


> On April 17, 2013, 1:50 a.m., Simeon Bird wrote:
> > common/regexpcache.cpp, line 102
> > <http://git.reviewboard.kde.org/r/109991/diff/1/?file=138503#file138503line102>
> >
> >     I'm slow and stupid...could you please comment what it is you are trying to do with this function? Ideally you could split it up into some smaller functions.

Generally it doesn't look good at first glance, but I'm not sure whether a lot of small functions will make it better. Regexp code and concatenation of multiple strings often may look ugly. I've added some comments and splitted first part into two separated functions. Is it better?


> On April 17, 2013, 1:50 a.m., Simeon Bird wrote:
> > common/regexpcache.cpp, line 199
> > <http://git.reviewboard.kde.org/r/109991/diff/1/?file=138503#file138503line199>
> >
> >     Why don't you use QString::split?

I've added comment.


> On April 17, 2013, 1:50 a.m., Simeon Bird wrote:
> > common/regexpcache.cpp, line 229
> > <http://git.reviewboard.kde.org/r/109991/diff/1/?file=138503#file138503line229>
> >
> >     This seems like a behaviour change, rather than an optimisation? Could you split it into a separate commit (git rebase is your friend here) and comment the intended purpose of the function?

Not really. I use full regexp format to join filters to make something like this: c(make|pp|vs|lass), but only wildcard syntax is supported in each filter, so behavior isn't changed.


On April 17, 2013, 1:50 a.m., Lukasz Olender wrote:
> > Thanks for the patch! Please could you add a few comments on the design of the optimised regexp, why it is faster, how it works, and so on? It really helps us to review it faster. 
> > 
> > Also, could you please add the test cases you link to the nepomuk tests directory? If you like, submit a separate patch adding them, as a test for regexp speed and correctness would be useful in itself. 
> >

Yes. I'll make an another patch when review of this will be finished. Thanks for response!


- Lukasz


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109991/#review31192
-----------------------------------------------------------


On April 18, 2013, 4:27 p.m., Lukasz Olender wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109991/
> -----------------------------------------------------------
> 
> (Updated April 18, 2013, 4:27 p.m.)
> 
> 
> Review request for Nepomuk and Vishesh Handa.
> 
> 
> Description
> -------
> 
> It's related with https://bugs.kde.org/show_bug.cgi?id=303654. 
> P.S. I accidentally deleted author's and license info in patch. Isolated performance tests are also uploaded to http://www.sendspace.com/file/mkihdp (previous link not always work). It's my first patch.
> 
> 
> This addresses bug 303654.
>     http://bugs.kde.org/show_bug.cgi?id=303654
> 
> 
> Diffs
> -----
> 
>   common/regexpcache.h d89f968 
>   common/regexpcache.cpp df45277 
> 
> Diff: http://git.reviewboard.kde.org/r/109991/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Lukasz Olender
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/nepomuk/attachments/20130418/a865220b/attachment.html>