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

[Simeon Bird]

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



common/regexpcache.h
<http://git.reviewboard.kde.org/r/109991/#comment23196>

    You do need to restore these



common/regexpcache.h
<http://git.reviewboard.kde.org/r/109991/#comment23195>

    Revert this change: the dangling _ is quite important!



common/regexpcache.cpp
<http://git.reviewboard.kde.org/r/109991/#comment23197>

    Please remove the extra space between QRegExp and &



common/regexpcache.cpp
<http://git.reviewboard.kde.org/r/109991/#comment23198>

    "Begin of pattern" is good Italian, but not so good English..."Beginning Pattern" would be better



common/regexpcache.cpp
<http://git.reviewboard.kde.org/r/109991/#comment23202>

    Why not just QString()?



common/regexpcache.cpp
<http://git.reviewboard.kde.org/r/109991/#comment23201>

    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.



common/regexpcache.cpp
<http://git.reviewboard.kde.org/r/109991/#comment23200>

    Why don't you use QString::split?



common/regexpcache.cpp
<http://git.reviewboard.kde.org/r/109991/#comment23199>

    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?


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. 


- Simeon Bird


On April 13, 2013, 12:56 p.m., Lukasz Olender wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109991/
> -----------------------------------------------------------
> 
> (Updated April 13, 2013, 12:56 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/20130417/a58e0173/attachment-0001.html>