<table><tr><td style="">dfaure requested changes to this revision.<br />dfaure added inline comments.<br />This revision now requires changes to proceed.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D12945">View Revision</a></tr></table><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12945#inline-67897">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kcoredirlister_benchmark.cpp:244</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">uint</span> <span class="n">hash</span><span style="color: #aa2211">=</span><span class="n">qHash</span><span class="p">(</span><span class="n">url</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">auto</span> <span class="n">it</span> <span style="color: #aa2211">=</span> <span class="n">std</span><span style="color: #aa2211">::</span><span class="n">lower_bound</span><span class="p">(</span><span class="n">lstItems</span><span class="p">.</span><span class="n">cbegin</span><span class="p">(),</span> <span class="n">lstItems</span><span class="p">.</span><span class="n">cend</span><span class="p">(),</span> <span class="n">hash</span><span class="p">,</span> <span class="n">lessThanHash</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">it</span> <span style="color: #aa2211">!=</span> <span class="n">lstItems</span><span class="p">.</span><span class="n">cend</span><span class="p">()</span> <span style="color: #aa2211">&&</span> <span class="n">it</span><span style="color: #aa2211">-></span><span class="n">item</span><span style="color: #aa2211">-></span><span class="n">url</span><span class="p">()</span> <span style="color: #aa2211">==</span> <span class="n">url</span><span class="p">)</span> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">As discussed in <a href="https://phabricator.kde.org/D10742" class="remarkup-link" target="_blank" rel="noreferrer">https://phabricator.kde.org/D10742</a>, this doesn't handle the case of hash collisions (this code assumes each URL gets a unique hash). So this commit needs to be updated as well, possibly by just removing this class if making it right is too much effort, for too slow code in the end?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12945#inline-67900">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kcoredirlister_benchmark.cpp:329</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #aa4000">template</span> <span style="color: #aa2211"><</span><span style="color: #aa4000">class</span> <span style="color: #00702a">T</span><span style="color: #aa2211">></span> <span style="color: #aa4000">void</span> <span class="n">createFiles</span><span class="p">(</span><span style="color: #aa4000">int</span> <span class="n">number</span><span class="p">)</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">number</tt> is always 50 so you could remove it as a parameter in all these methods and just use a static const int for instance.</p>
<p style="padding: 0; margin: 8px;">It's more dangerous to have it as a method parameter: if one caller passes a different number, the benchmarks won't be comparable anymore...</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12945#inline-67901">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kcoredirlister_benchmark.cpp:376</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QBENCHMARK</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">for</span> <span class="p">(</span><span style="color: #aa4000">int</span> <span class="n">i</span><span style="color: #aa2211">=</span><span style="color: #601200">0</span><span class="p">;</span> <span class="n">i</span><span style="color: #aa2211"><</span><span style="color: #601200">5000</span><span class="p">;</span> <span class="n">i</span><span style="color: #aa2211">++</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">data</span><span class="p">.</span><span class="n">findByUrl</span><span class="p">(</span><span class="n">u1</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">QBENCHMARK takes care of repeating as many times as necessary, so this for loop isn't needed, is it?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12945#inline-67902">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kcoredirlister_benchmark.cpp:384</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QUrl</span><span style="color: #aa2211">::</span><span class="n">fromLocalFile</span><span class="p">(</span><span class="n">QLatin1String</span><span class="p">(</span><span style="color: #766510">"/home/user/Folder1/SubFolder2/a4505.txt"</span><span class="p">)));</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QCOMPARE</span><span class="p">(</span><span class="n">data</span><span class="p">.</span><span class="n">findByUrl</span><span class="p">(</span><span class="n">QUrl</span><span style="color: #aa2211">::</span><span class="n">fromLocalFile</span><span class="p">(</span><span class="n">QLatin1String</span><span class="p">(</span><span style="color: #766510">"/home/user/Folder1/SubFolder2/a4505.txt"</span><span class="p">)))</span><span style="color: #aa2211">-></span><span class="n">url</span><span class="p">(),</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QUrl</span><span style="color: #aa2211">::</span><span class="n">fromLocalFile</span><span class="p">(</span><span class="n">QLatin1String</span><span class="p">(</span><span style="color: #766510">"/home/user/Folder1/SubFolder2/a4505.txt"</span><span class="p">)));</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">copy/paste errors? This is looking up the same URL three times.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R241 KIO</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D12945">https://phabricator.kde.org/D12945</a></div></div><br /><div><strong>To: </strong>jtamate, dfaure, Frameworks<br /><strong>Cc: </strong>kde-frameworks-devel, michaelh, ngraham, bruns<br /></div>