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

[Simeon Bird]

> 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?
> 
> Lukasz Olender wrote:
>     I've added comment.

Right...but I think you should use this function:

QStringList QString::split ( const QRegExp & rx, SplitBehavior behavior = KeepEmptyParts ) const

with a regex that means "split on any character NOT []\" 
(I can't remember offhand what that looks like in regex, but you can look it up).


> 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()?
> 
> Lukasz Olender wrote:
>     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()?

Yes - it is much clearer C++ idiom. Maybe if you were writing C you would put (const char *) NULL, but it's not really appropriate here. It is understood that QString() is empty.


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. 
> >
> 
> Lukasz Olender wrote:
>     Yes. I'll make an another patch when review of this will be finished. Thanks for response!

You're welcome - actually though it would be really good if you could post the testcase patch now - I wanted to run the test for review of this patch.


- Simeon


-----------------------------------------------------------
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/20130420/b5ce6dad/attachment.html>