<table><tr><td style="">kfunk requested changes to this revision.<br />kfunk added a comment.<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/D4020" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>Looks good to me *in general*, but this needs more refactoring IMO (too much logic duplication in Parameters).</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;" rel="noreferrer">@antonanikin</a> Would like your input as well if possible</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/D4020#inline-16190" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">parameters.h:55</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: #d0ffd0;"> <span style="color: #74777d">// project settings.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="n">QString</span> <span class="n">m_filePath</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Lots of code/logic duplication here & below between Parameters & PerProjectSettings. => Difficult to maintain.</p>
<p style="padding: 0; margin: 8px;">It looks like all this isn't really necessarily to keep around. We already have PerProjecSettings for this, no? Job only needs m_dumpConfig. Maybe that state could be rather a property on Job itself? Instead of passing Parameters to the Job, pass the executable path + command line yourself? Or just set it from the outside?</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/D4020#inline-16189" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">perprojectconfigpage.cpp:46</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; "> <span class="n">m_availableChecksModel</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">QStringListModel</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="bright"></span><span class="n"><span class="bright">ui</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">-></span></span><span class="bright"></span><span class="n"><span class="bright">c</span>heck<span class="bright">ListView</span></span><span style="color: #aa2211">-></span><span class="n">set<span class="bright">Model</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">m_a</span>vail<span class="bright">able</span>Checks<span class="bright">Model</span></span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="bright"></span><span class="n"><span class="bright">m_availableC</span>heck<span class="bright">sModel</span></span><span style="color: #aa2211">-></span><span class="n">set<span class="bright">StringList</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">m_underlineA</span>vailChecks</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Better: <tt style="background: #ebebeb; font-size: 13px;">new QStringListModel(this);</tt> -- otherwise leaks</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/D4020#inline-16188" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">perprojectsettings.kcfg:16</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: #d0ffd0;"> </entry>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <entry name="AnaliseTempDtors" key="AnaliseTempDtors" type="Bool">
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <default>false</default>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Typo: Analise</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/D4020#inline-16191" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">job.h:42</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; "><span style="color: #a0a000">public</span><span class="p">:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="bright"></span><span style="color: #74777d"><span class="bright">/**</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"><span style="color: #74777d"> * \class</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"><span style="color: #74777d"> * \brief command line parameters.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"><span style="color: #74777d"> */</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span style="color: #aa4000">struct</span> <span class="n">Parameters</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="n">QString</span> <span class="n">projectRootDir</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="n">QString</span> <span class="n">executablePath</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="n">QString</span> <span class="n">filePath</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="n">QString</span> <span class="n">buildDir</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="n">QString</span> <span class="n">additionalParameters</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="n">QString</span> <span class="n">analiseTempDtors</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="n">QString</span> <span class="n">enabledChecks</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="n">QString</span> <span class="n">useConfigFile</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="n">QString</span> <span class="n">dumpConfig</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="n">QString</span> <span class="n">enableChecksProfile</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="n">QString</span> <span class="n">exportFixes</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="n">QString</span> <span class="n">extraArgs</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="n">QString</span> <span class="n">extraArgsBefore</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="n">QString</span> <span class="n">autoFix</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="n">QString</span> <span class="n">autoFixError</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="n">QString</span> <span class="n">headerFilter</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="n">QString</span> <span class="n">lineFilter</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="n">QString</span> <span class="n">listChecks</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="n">QString</span> <span class="n">checkSystemHeaders</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="p">};</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;">
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="n">Job</span><span class="p">(</span><span style="color: #aa4000">const</span> <span class="n">Parameters</span><span style="color: #aa2211">&</span> <span class="n">params</span><span class="p">,</span> <span class="n">QObject</span><span style="color: #aa2211">*</span> <span class="n">parent</span> <span style="color: #aa2211">=</span> <span class="n">nullptr</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="bright"></span><span class="n"><span class="bright">Job</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #aa4000"><span class="bright">const</span></span><span class="bright"> </span><span class="n"><span class="bright">Parameters</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">&</span></span><span class="bright"> </span><span class="n"><span class="bright">params</span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span class="n"><span class="bright">QObject</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">*</span></span><span class="bright"> </span><span class="n"><span class="bright">parent</span></span><span class="bright"></span><span class="p"><span class="bright">);</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa2211">~</span><span class="n">Job</span><span class="p">()</span> <span class="n">override</span><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;">parent = nullptr</tt> to be consistent with interface from base class.</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/D4020#inline-16192" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">replacementparser.h:91</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; "> <span class="n">ReplacementParser</span><span class="p">()</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">default</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="n">explicit</span> <span style="color: #004012">ReplacementParser</span><span class="p">(<span class="bright"></span></span><span class="bright"></span><span style="color: #aa4000"><span class="bright">const</span></span><span class="bright"> </span><span class="n">QString<span class="bright"></span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">&</span></span> <span class="n">yaml_file</span><span class="p">,</span> <span class="bright"></span><span style="color: #aa4000"><span class="bright">const</span></span><span class="bright"> </span><span class="n">QString<span class="bright"></span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">&</span></span> <span class="n">source_file</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="n">explicit</span> <span style="color: #004012">ReplacementParser</span><span class="p">(</span><span class="n">QString</span> <span class="n">yaml_file</span><span class="p">,</span> <span class="n">QString</span> <span class="n">source_file</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">void</span> <span style="color: #004012">parse</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Style: Keep const-refs + use camelCase for var names</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R218 KDev Clang-Tidy Support</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D4020" rel="noreferrer">https://phabricator.kde.org/D4020</a></div></div><br /><div><strong>EMAIL PREFERENCES</strong><div><a href="https://phabricator.kde.org/settings/panel/emailpreferences/" rel="noreferrer">https://phabricator.kde.org/settings/panel/emailpreferences/</a></div></div><br /><div><strong>To: </strong>coliveira, antonanikin, tcanabrava, KDevelop, kfunk<br /><strong>Cc: </strong>kdevelop-devel<br /></div>