<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/124139/">https://git.reviewboard.kde.org/r/124139/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 22nd, 2015, 7:41 p.m. UTC, <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;">Hey Sergey. First of all - nice! I've recently seen that new Qt code wants <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">-fPIC</code> or <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">-fPIE</code> and otherwise reports a <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">#error</code> which we then add as a problem to every translation unit - this patch will help here!</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The code review below shows some minor nitpicks. I'm not giving you a ship-it yet though because I really would like to see some improvements on the UI part:</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;">project settings</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">I dislike that it says "Clang" here - yes we do use it, but no this is not the compiler and people will mix it up. This is just about our Clang language backend, not about what we will pass to the compiler.</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Also, this duplicates some stuff with the custom includes/defines, no? Can't we add custom arguments there already? Why can this not be reused?</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">The dot as a root project path is not nice, I think. Could you maybe unalias relative paths there?</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">the standard is given in the combobox <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">and</em> repeated in the lineedit below - why?</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">session settings</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">the tab with only one check box is a waste of space.</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">could we maybe have a "per-language" section in the "Language Support" instead of adding another settings page?</li>
</ol></pre>
</blockquote>
<p>On July 2nd, 2015, 7:31 p.m. UTC, <b>Sergey Kalinichev</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;">Hi Milian, thanks for the feedback.
2. Maybe "Parser settings", then?
3. Yes we can, but that config page called "Custom Defines and Includes". Adding parser arguments there will be a bit confusing imo.
So I'm actually thinking about moving all that logic regarding language standards to kdev-clang, and exposing the CompilerProvider through DaIM - to set language standard to compilers.
5. Most of the time you change standard (e.g. the project you work on isn't c++11). We could use just the line edit to do it, but not everyone knows how to set the standard there manually, also it's easy to misspell.
Actually I'm thinking about adding default and advanced modes. Where the combobox is visible in the default mode, and the lineedit in the advanced mode.
7. More stuff can be added later on...
8. You mean something like adding tabs dynamically to the "Language Support" config page based on loaded language plugins?</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;"><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;">Maybe just "C/C++ parser"</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">or just have a combobox for the standard and a lineedit for everything else, except the standard </li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">but currently there is nothing there and it looks very bad.</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">yes something like that</li>
</ol></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 22nd, 2015, 7:41 p.m. UTC, <b>Milian Wolff</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/124139/diff/2/?file=380874#file380874line196" style="color: black; font-weight: bold; text-decoration: underline;">clangsettings/clangsettingsmanager.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">196</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">auto</span> <span class="n">list</span> <span class="o">=</span> <span class="n">parserOptions</span><span class="p">.</span><span class="n">split</span><span class="p">(</span><span class="sc">' '</span><span class="p">,</span> <span class="n">QString</span><span class="o">::</span><span class="n">SkipEmptyParts</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">juk, this is ugly but OK for now as it's (hopefully) not called often (or is it, once per file?). Anyhow, add a TODO note to improve this in the future. I hate it, that there is no simple API to split into a QVector :-/</p></pre>
</blockquote>
<p>On July 2nd, 2015, 7:31 p.m. UTC, <b>Sergey Kalinichev</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 is called once per parse session. Still I don't get what's wrong with it and what should be added to the TODO note?</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;">it's very inefficient. you first allocate a list and then convert it to a vector.</p></pre>
<br />
<p>- Milian</p>
<br />
<p>On June 21st, 2015, 6:37 p.m. UTC, 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 June 21, 2015, 6:37 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdev-clang
</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;">This adds two pages: One for session settings (code-completion, assistants), another one for project settings (parser command-line arguments).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Known issue: now we can set language standard in two places: DaIM - for deducing standard macros/includes and here - for the internal parser.
To solve this problem we can copy/move compilers infrastructure to kdev-clang. Also we can expose the compiler provider interface through DaIM (which is a much better solution imo). Suggestions?</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>CMakeLists.txt <span style="color: grey">(875172a)</span></li>
<li>clangparsejob.cpp <span style="color: grey">(bea4fc0)</span></li>
<li>clangsettings/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>clangsettings/clangconfigpage.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>clangsettings/clangconfigpage.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>clangsettings/clangprojectconfig.kcfg <span style="color: grey">(PRE-CREATION)</span></li>
<li>clangsettings/clangprojectconfig.kcfgc <span style="color: grey">(PRE-CREATION)</span></li>
<li>clangsettings/clangsettingsmanager.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>clangsettings/clangsettingsmanager.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>clangsettings/clangsettingsplugin.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>clangsettings/clangsettingsplugin.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>clangsettings/configwidget.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>clangsettings/configwidget.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>clangsettings/configwidget.ui <span style="color: grey">(PRE-CREATION)</span></li>
<li>clangsettings/kdevclangsettings.json <span style="color: grey">(PRE-CREATION)</span></li>
<li>clangsettings/pathsmodel.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>clangsettings/pathsmodel.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>clangsettings/sessionsettings/sessionconfig.kcfg <span style="color: grey">(PRE-CREATION)</span></li>
<li>clangsettings/sessionsettings/sessionconfig.kcfgc <span style="color: grey">(PRE-CREATION)</span></li>
<li>clangsettings/sessionsettings/sessionconfigskeleton.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>clangsettings/sessionsettings/sessionsettings.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>clangsettings/sessionsettings/sessionsettings.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>clangsettings/sessionsettings/sessionsettings.ui <span style="color: grey">(PRE-CREATION)</span></li>
<li>codecompletion/context.cpp <span style="color: grey">(f26921f)</span></li>
<li>codegen/adaptsignatureassistant.cpp <span style="color: grey">(2cdd34b)</span></li>
<li>duchain/CMakeLists.txt <span style="color: grey">(e07ef70)</span></li>
<li>duchain/clangparsingenvironment.h <span style="color: grey">(2e4ea8b)</span></li>
<li>duchain/clangparsingenvironment.cpp <span style="color: grey">(20a2dbf)</span></li>
<li>duchain/parsesession.h <span style="color: grey">(74999de)</span></li>
<li>duchain/parsesession.cpp <span style="color: grey">(563d227)</span></li>
<li>duchain/unknowndeclarationproblem.cpp <span style="color: grey">(908a518)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/124139/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/06/21/f4bb2220-dc63-478d-b423-f31434496afb__project_settings.png">project settings.png</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/06/21/e1f59a46-f0b4-46fa-9f19-ffca4b887cf5__session_settings.png">session settings.png</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>