<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/121777/">https://git.reviewboard.kde.org/r/121777/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 2nd, 2015, 9:34 p.m. MSK, <b>Nicolai Hähnle</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This seems like a <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">very</em> useful improvement, but the design rubs me the wrong way in several places, which can be summed up with the following observations:</p>
<ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Different compilers support different standards.</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">The standard which is in use is not a property of the <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">compiler</em>, but a property of <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">individual source files</em> within a project.</li>
</ol>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Note how the code design is incompatible with these two observations in a number of important ways:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">1a. There is a globally hardcoded list/enum of standards even though not every compiler will support every standard. I would remove this list and instead add a method <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QList<QString> ICompiler::supportedStandards() const</code>.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">1b. The list of standards changes; in fact, the list of standards is already out of date, since C++14 is a thing.</p>
<ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">ICompiler::setStandard() is really bad and dangerous design. SettingsManager::currentCompiler(...) will hand out pointers to a global set of compilers. So it is easily possible that two different projects (or two different parts of the same project!) that use different standards will end up holding pointers to the same ICompiler object, resulting in terrible confusion.</li>
</ol>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For example, note how the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">GccLikeCompiler::defines()</code> implementation caches the result once it has been obtained, and subsequent changes to the standard will simply be ignored.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I suggest to eliminate <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ICompiler::setStandard()</code>, and instead pass the standard as a parameter to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ICompiler::defines(const QString& standard)</code> and <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ICompiler::includes(const QString& standard)</code>.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I see two possible alternatives:</p>
<ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Create another class that is layered on top of ICompiler that plays the role of "compiler + standard", or</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Do away with the current approach entirely and make the standard a property of the ICompiler. In other words, instead of giving the user the choice of "GCC" or "Clang" or whatever compilers, give them a choice between "GCC C++03", "GCC C++11", "GCC C++14", "Clang C++03", etc. compilers. (Personally, I prefer this last alternative because it is the simplest in terms of code changes.)</li>
</ol></pre>
</blockquote>
<p>On January 3rd, 2015, 6:28 p.m. MSK, <b>Milian Wolff</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Excellent review Nicolai, I couldn't say it any better. I also say we should simply add "GCC C++03" or similar "compilers". That is simple to implement code-wise and easy to grasp, UI-wise.</p></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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">1a. There is a globally hardcoded list/enum of standards even though not every compiler will support every standard. I would remove this list and instead add a method ICompiler::supportedStandards() const.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That's a valid point. I'll make it a virtual method then</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">1b. The list of standards changes; in fact, the list of standards is already out of date, since C++14 is a thing.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That's true too. Still only recent versions of clang/gcc support it. So I thought it's too early to add it here. And yes I agree that we should query compiler for supported standards instead.
Any ideas how to make that?
I saw: gcc -v --help. But seems like clang doesn't support that...
Also we could compile some file with different standards and see the result.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">SettingsManager::currentCompiler(...) will hand out pointers to a global set of compilers.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Indeed. Thanks for reminding that! I completely forgot about it.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In other words, instead of giving the user the choice of "GCC" or "Clang" or whatever compilers, give them a choice between "GCC C++03", "GCC C++11", "GCC C++14", "Clang C++03", etc. compilers. (Personally, I prefer this last alternative because it is the simplest in terms of code changes.)</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, I think I'll do it that way. The only thing that troubles me is that it works well only if you have 2 compilers. But if you have 4-5 (gcc, clang, android-gcc, mingw, ...) there'll be like ~30 choises. That's not very good IMO. So I'm planning to make just 2 entries for each compiler, e,g,: "GCC c++11", "GCC c99" e.t.c. All other standards could be aded manually. What do you think?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">BTW great review, thanks!</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 2nd, 2015, 9:34 p.m. MSK, <b>Nicolai Hähnle</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="https://git.reviewboard.kde.org/r/121777/diff/1/?file=337642#file337642line74" style="color: black; font-weight: bold; text-decoration: underline;">languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">ProjectPathsWidget::ProjectPathsWidget( QWidget* parent )</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">74</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">connect</span><span class="p">(</span><span class="n">ui</span><span class="o">-></span><span class="n">compilersWidget</span><span class="p">,</span> <span class="o">&</span><span class="n">CompilersWidget</span><span class="o">::</span><span class="n">compilerChanged</span><span class="p">,</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">75</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">this</span><span class="p">,</span> <span class="o">&</span><span class="n">ProjectPathsWidget</span><span class="o">::</span><span class="n">compilerChanged</span><span class="p">);</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">76</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As far as I can tell, this is the only "call-site" to ProjectPathsWidget::compilerChanged. I don't quite understand what the code is trying to do, but either this must stay, or compilerChanged must be removed as well.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I have no clue why I did that. Thanks again!</p></pre>
<br />
<p>- Sergey</p>
<br />
<p>On January 1st, 2015, 3:10 p.m. MSK, Sergey Kalinichev wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDevelop.</div>
<div>By Sergey Kalinichev.</div>
<p style="color: grey;"><i>Updated Jan. 1, 2015, 3:10 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdevelop
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Now we can choose different language standards for computing standard include directories/defined macros.
Also this feature is very useful for the kdev-clang plugin (Now it's possible to parse plain C projects).</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.ui <span style="color: grey">(3e413b3)</span></li>
<li>languages/plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp <span style="color: grey">(3643e78)</span></li>
<li>languages/plugins/custom-definesandincludes/compilerprovider/gcclikecompiler.cpp <span style="color: grey">(bc99d8d)</span></li>
<li>languages/plugins/custom-definesandincludes/compilerprovider/icompiler.h <span style="color: grey">(3627ebd)</span></li>
<li>languages/plugins/custom-definesandincludes/compilerprovider/icompiler.cpp <span style="color: grey">(0bf6a44)</span></li>
<li>languages/plugins/custom-definesandincludes/compilerprovider/settingsmanager.cpp <span style="color: grey">(229f456)</span></li>
<li>languages/plugins/custom-definesandincludes/kcm_widget/definesandincludesconfigpage.cpp <span style="color: grey">(75f224d)</span></li>
<li>languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.h <span style="color: grey">(446ddb5)</span></li>
<li>languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.cpp <span style="color: grey">(5b34e75)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/121777/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>