<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/109991/">http://git.reviewboard.kde.org/r/109991/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 20th, 2013, 4:14 a.m. UTC, <b>Simeon Bird</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/109991/diff/2/?file=139540#file139540line108" style="color: black; font-weight: bold; text-decoration: underline;">common/regexpcache.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">102</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QPair</span><span class="o"><</span><span class="kt">size_t</span><span class="p">,</span> <span class="n">QString</span><span class="o">></span> <span class="n">maxBegValue</span> <span class="o">=</span> <span class="n">QPair</span><span class="o"><</span><span class="kt">size_t</span><span class="p">,</span> <span class="n">QString</span><span class="o">></span><span class="p">(</span> <span class="mi">0</span><span class="p">,</span> <span class="mi">0</span> <span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Do you really need to initialise this?</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I'm not sure - size_t is always initialized '0' by default? </pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 20th, 2013, 4:14 a.m. UTC, <b>Simeon Bird</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/109991/diff/2/?file=139540#file139540line119" style="color: black; font-weight: bold; text-decoration: underline;">common/regexpcache.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">113</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">QPair</span><span class="o"><</span><span class="kt">size_t</span><span class="p">,</span> <span class="n">QString</span><span class="o">></span> <span class="n">RegExpCache</span><span class="o">::</span><span class="n">getMostCommonCharAtEnd</span><span class="p">(</span> <span class="k">const</span> <span class="n">QStringList</span><span class="o">&</span> <span class="n">filters</span> <span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This is basically the same function as getMostCommonCharAtBeg, and they are always called together, so why don't you unify them?</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I've joined them into new one.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 20th, 2013, 4:14 a.m. UTC, <b>Simeon Bird</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/109991/diff/2/?file=139540#file139540line201" style="color: black; font-weight: bold; text-decoration: underline;">common/regexpcache.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">195</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">int</span> <span class="n">indexOfBracket</span> <span class="o">=</span> <span class="n">innerPattern</span><span class="p">.</span><span class="n">indexOf</span><span class="p">(</span> <span class="n">QChar</span><span class="p">(</span> <span class="sc">')'</span> <span class="p">)</span> <span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Should be ( I think</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">No, I've added comment with explanation in new diff.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 20th, 2013, 4:14 a.m. UTC, <b>Simeon Bird</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/109991/diff/2/?file=139540#file139540line207" style="color: black; font-weight: bold; text-decoration: underline;">common/regexpcache.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">201</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">int</span> <span class="n">i1</span> <span class="o">=</span> <span class="n">innerPattern</span><span class="p">.</span><span class="n">indexOf</span><span class="p">(</span> <span class="s">")"</span><span class="p">,</span> <span class="n">innerPattern</span><span class="p">.</span><span class="n">indexOf</span><span class="p">(</span> <span class="n">QChar</span><span class="p">(</span> <span class="sc">')'</span> <span class="p">)</span> <span class="p">)</span> <span class="o">+</span> <span class="mi">1</span> <span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Not at all, I'm trying to remove outermost (), I've also added comment.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 20th, 2013, 4:14 a.m. UTC, <b>Simeon Bird</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I've added it. It's about 5 times faster using standard Nepomuk's filters on my machine. Actually, combining them against just joining with "|" makes all of the difference. My solution will be unfortunately about two times slower if I'll just join all those filters by "|" and let QRegExp do the job (it's easy to check by setting minOccur variable in createPattern method to value bigger than number of filters). I've added this information in comment to createPattern method (now it's named groupPatterns). Probably it will be slower if filters won't have any common patterns (for example if user will have a number of ignored files and no common filters at all). In such a situation, we can switch to old behavior, but it's quite hard to check when it's worth switching, so I'm leaving it as it is. I realize there might be a need to handle also that cases.</pre>
<br />
<p>- Lukasz</p>
<br />
<p>On April 22nd, 2013, 10:19 p.m. UTC, Lukasz Olender wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Nepomuk and Vishesh Handa.</div>
<div>By Lukasz Olender.</div>
<p style="color: grey;"><i>Updated April 22, 2013, 10:19 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=303654">303654</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>common/regexpcache.cpp <span style="color: grey">(df45277)</span></li>
<li>common/regexpcache.h <span style="color: grey">(d89f968)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/109991/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>