----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109991/#review31314 -----------------------------------------------------------
common/regexpcache.h <http://git.reviewboard.kde.org/r/109991/#comment23325> Could you get rid of the extra whitespace, please? common/regexpcache.h <http://git.reviewboard.kde.org/r/109991/#comment23329> I would rename this to something like "groupPatterns" common/regexpcache.h <http://git.reviewboard.kde.org/r/109991/#comment23330> I would rename this to something like "splitAndEscapeString common/regexpcache.cpp <http://git.reviewboard.kde.org/r/109991/#comment23326> Do you really need to initialise this? common/regexpcache.cpp <http://git.reviewboard.kde.org/r/109991/#comment23327> Why don't you keep track of the maximum value in the earlier loop, to save looping over things twice? common/regexpcache.cpp <http://git.reviewboard.kde.org/r/109991/#comment23328> This is basically the same function as getMostCommonCharAtBeg, and they are always called together, so why don't you unify them? common/regexpcache.cpp <http://git.reviewboard.kde.org/r/109991/#comment23334> Put one single Q_ASSERT at the beginning of the function common/regexpcache.cpp <http://git.reviewboard.kde.org/r/109991/#comment23331> but uccurs -> but it occurs common/regexpcache.cpp <http://git.reviewboard.kde.org/r/109991/#comment23336> Should be ( I think common/regexpcache.cpp <http://git.reviewboard.kde.org/r/109991/#comment23335> I think you mean QChar '(' again here, don't you? You seem here to be trying to remove empty (), and I think there is an easier way to do that. If this is not what you mean to do, could you comment it? Could you also add to the commit message a comment on the sort of performance gains this patch produces? ie, is it O(10%), or an order of magnitude? Also, how much of the improvement is due to the combining of filters in createPattern?, and how much just to rolling the RegEx into one long (||||) one? - Simeon Bird 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 > >
_______________________________________________ Nepomuk mailing list [email protected] https://mail.kde.org/mailman/listinfo/nepomuk
