<table><tr><td style="">kossebau accepted this revision.<br />kossebau added a comment.<br />This revision is now accepted and ready to land.
</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/D14288">View Revision</a></tr></table><br /><div><div><p>Another scan over the code, no show-stoppers seen (though I might be blind by now). I am a litle unsure about all the UI strings, they might see some more brush over. Though then the same is true for strings in the current KDevelop codebase, so not such a bugger.<br />
From the functionality POV, I would have some proposals for changes, but as shown by my usage of the plugin for KDevelop's own codebase, the plugin can be used successfully for its purpose as it is. And with feedback from users of 5.3 then things could be developed further.</p>
<p>So when it comes to me, I would be fine by merging the patch as is (ideally with last nitpicks solved). Any further clean-up if needed could then be done inside the 5.3 branch.</p>
<p>I propose to still wait with the string freeze until Thursday, so people building from master/5.3 have two days of chance to give feedback on the UI. Given <a href="https://phabricator.kde.org/p/brauch/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@brauch</a> passed settings the string freeze day here on to me, let's have it like that :)</p>
<p><a href="https://phabricator.kde.org/p/antonanikin/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@antonanikin</a> So if <a href="https://phabricator.kde.org/p/kfunk/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@kfunk</a> <a href="https://phabricator.kde.org/p/mwolff/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@mwolff</a> or <a href="https://phabricator.kde.org/p/apol/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@apol</a> (as maintainers around currently) do not have any further comments/objects in the next few hours, please merge this to the 5.3 branch tonight :)<br />
<a href="https://phabricator.kde.org/p/pino/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@pino</a> Also would be glad for some last UI string review by you if needed.</p></div></div><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/D14288#inline-79982">View Inline</a><span style="color: #4b4d51; font-weight: bold;">projectconfigpage.ui:56</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);"> <property name="toolTip">
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <string>Won't emit warnings for non-Qt files, or in other words, if -DQT_CORE_LIB is missing</string>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> </property>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">All tooltip texts should end with a full-stop. (hm, cannot find the respective mention in any hig guide, just remember it is like that). <br />
Also "Won't" -> "Do not".</p>
<p style="padding: 0; margin: 8px;">For some general guidelines see <a href="https://hig.kde.org/style/writing/wording.html" class="remarkup-link" target="_blank" rel="noreferrer">https://hig.kde.org/style/writing/wording.html</a></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/D14288#inline-79984">View Inline</a><span style="color: #4b4d51; font-weight: bold;">projectconfigpage.ui:66</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);"> <property name="toolTip">
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <string>Turns off checks not compatible with Qt 4</string>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> </property>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">"Disable checks not compatible with Qt 4.,"</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/D14288#inline-79987">View Inline</a><span style="color: #4b4d51; font-weight: bold;">job.cpp:263</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">setPercent</span><span class="p">(</span><span style="color: #601200">100</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">postProcessStdout</span><span class="p">({</span><span class="n">QString</span><span class="p">(</span><span style="color: #766510">"Elapsed time: %1 s."</span><span class="p">).</span><span class="n">arg</span><span class="p">(</span><span class="n">m_timer</span><span style="color: #aa2211">-></span><span class="n">elapsed</span><span class="p">()</span><span style="color: #aa2211">/</span><span style="color: #601200">1000.0</span><span class="p">)});</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">QString(...) _> QStringLiteral(...)</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/D14288#inline-79989">View Inline</a><span style="color: #4b4d51; font-weight: bold;">problemmodel.cpp:82</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="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">auto</span> <span style="color: #a0a000">problem</span> <span class="p">:</span> <span class="n">qAsConst</span><span class="p">(</span><span class="n">m_problems</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 style="color: #aa4000">if</span> <span class="p">(</span><span class="n">newProblem</span><span style="color: #aa2211">-></span><span class="n">source</span><span class="p">()</span> <span style="color: #aa2211">==</span> <span class="n">problem</span><span style="color: #aa2211">-></span><span class="n">source</span><span class="p">()</span> <span style="color: #aa2211">&&</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">What I mean what I prefer is this:</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">for (auto& problem : qAsConst(m_problems)) {</pre></div>
<p style="padding: 0; margin: 8px;">Reason is that <tt style="background: #ebebeb; font-size: 13px;">KDevelop::IProblem::Ptr</tt> is not just a plain raw pointer, but a slightly more complex datatype with a non-trivial copy constructor, which would be used in case of just <tt style="background: #ebebeb; font-size: 13px;">auto problem</tt>).<br />
By using a reference to the Ptr object we spare the invocations of that copy constructor call per loop step (cmp. <tt style="background: #ebebeb; font-size: 13px;">QExplicitlySharedDataPointer(const QExplicitlySharedDataPointer<X> &o)</tt> code), which saves again some cpy cycles. Not a big gain here indeed, but keeping this as pattern ensures that code in other places which are more a hotpath default to the less expensive variant.<br />
(BTW, clazy warns about that in the range-loop check :P )</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/D14288#inline-79990">View Inline</a><span style="color: #4b4d51; font-weight: bold;">problemmodel.cpp:105</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">for</span> <span class="p">(</span><span style="color: #aa4000">auto</span> <span style="color: #a0a000">problem</span> <span class="p">:</span> <span class="n">problems</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 style="color: #aa4000">if</span> <span class="p">(</span><span class="n">problemExists</span><span class="p">(</span><span class="n">problem</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;">same auto& problem</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R32 KDevelop</div></div></div><br /><div><strong>BRANCH</strong><div><div>kdev_clazy</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D14288">https://phabricator.kde.org/D14288</a></div></div><br /><div><strong>To: </strong>antonanikin, KDevelop, kossebau<br /><strong>Cc: </strong>arrowd, mwolff, apol, kfunk, brauch, pino, kossebau, kdevelop-devel, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight<br /></div>