<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 15th, 2015, 10:40 p.m. UTC, <b>Kevin Funk</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;">I just briefly skimmed the patch. I think we need to overthink this completely:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What do we actually want?
- We'd like to be able to select the language standard for our <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">internal</em> parser.
- This has nothing to do with binaries installed on the system
- This has nothing to do with the specific compiler either (MSVC, GCC, ...)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Proposal:
- Add this setting to a language-specific settings page instead. I.e. a "C/C++/Obj" settings page
- I already have multiple other ideas that I could add to this page afterwards
(like making our auto-complete options configurable, for instance whether to add 'override' to a virtual function overload, etc.)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So in fact I'm proposing to move the "language standard" feature directly to kdev-clang instead. </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Opions?</p></pre>
</blockquote>
<p>On January 18th, 2015, 12:48 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;"><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;">Add this setting to a language-specific settings page instead. I.e. a "C/C++/Obj" settings page. So in fact I'm proposing to move the "language standard" feature directly to kdev-clang instead. </p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There are a couple of reasons, why it's IMO not very good:
a) There is no such page yet.
b) The oldcpp can use that standard feature too. Actually I've seen a bug report about not configurable include directories (i.e. c++11 only)
c) Even if we do that. Then we have to move compiler setting to that page too. Otherwise it'd be inconvenient to select compiler on one page and standard on another.
d) Also, is this "C/C++/Obj" settings page per project? If it's project wide, then ok, otherwise it won't work. As language standard can be set per project.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So I'm suggesting to leave it as is. And give it more thinking when we finally get rid of oldcpp plugin.</p></pre>
</blockquote>
<p>On January 18th, 2015, 6:28 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;">a) the page can be created
b) oldcpp is not supporting different standards anyways, and will die sooner or later. so thats not an argument
c) this is a valid point we need to figure out how to handle
d) also true</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">anyhow, any comments to what I said? simplifying it all by letting the user specify the command line?</p></pre>
</blockquote>
<p>On January 19th, 2015, 10:27 a.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;">I don't know. Obviously not all users know what commands to pass. Also it's simple to misspell the options.
And I think that auto-detection feature still makes sense, as in most cases users want it to work out of the box without spending time on configuring compilers.</p></pre>
</blockquote>
<p>On January 19th, 2015, 11:25 a.m. UTC, <b>Kevin Funk</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;">c) True. But right now the Compiler Settings page is confusing already. Adding more options doesn't make it better. The user doesn't know what he's configuring. Is he configuring the compiler used to built his project or the internal parser?
We need to design that UI carefully, as it's already starting to become confusing IMO.
d) Yes. I was thinking about making this project-specific.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">e) Now that this feature would be added to the IADM settings page => oldcpp would ignore that settings => confuses users as well.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Honestly, I'm unsure how to handle all this, need to think about it more.</p></pre>
</blockquote>
<p>On January 21st, 2015, 2:05 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;"><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;">Adding more options doesn't make it better.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Maybe, but it makes it more flexible. Actually this patch doesn't add additional options. Before you could choose: "gcc", "clang". Now: "gcc c99", "gcc c++11" e.t.c. That's the only difference.</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;">The user doesn't know what he's configuring. Is he configuring the compiler used to built his project or the internal parser?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Are there really such users? I mean, the page is called "Custom defines and includes". Also there is a text label explaining what it's for. How could anyone confuse it with a compiler used for building the project?
Actually it'd be great to use the same compiler for both (project building and parser), but it's not always possible.</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;">Now that this feature would be added to the IADM settings page => oldcpp would ignore that settings => confuses users as well.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I wouldn't say so. Yes, due to it bugs oldcpp can't parse all plain C projects. But this setting affects standard include directories/macros used by the oldcpp, so it doesn't ignore this feature.</p></pre>
</blockquote>
<p>On January 21st, 2015, 2:09 p.m. UTC, <b>Craig Tenenbaum</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;">I think I may have some ideas about a potential compromise. I'm kind of motivated to see progress on this issue because I work with a codebase of mixed C++03, C++11, and C++14 code, where our leadership is unduly paranoid about simply building up our legacy, C++03 code segments to later specs. So, this can be kind of a perilous environment for really robust parsing/highlighting. Not to mention, I had been kind of cheating kdev-clang into properly parsing C++14 library features (language features like <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">decltype(auto)</code> and <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">auto</code>-return functions with no trailing specifier still don't work without proper C++14 support) by adding <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">__cplusplus 201403L</code> to the IADM, but this finally broke (as I always knew it would) with some of the recent performance enhancements to import processing.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So, okay, the compromise: in CMake-based projects, KDevelop already (I believe) passes <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">CMAKE_EXPORT_COMPILE_COMMANDS=ON</code>, which generates a <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">compile_commands.json</code> file that can be used by Clang's CompilationDatabase functionality to automatically extract per-file/per-configuration compiler arguments. I think this would be the ideal way to handle this particular problem in the instances where a project is CMake-based and doesn't use a generator which doesn't support compilation databases (not all do, I'm under the distinct impression Ninja only just recently had this added; I'm on CMake 2.8.12.2 myself and I seem to be able to generate compilation databases with Ninja just fine).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The above approach has the extreme benefit of making it possible for a mixed-standard project to be properly parsed and flagged for potential issues/errors in exactly the fashion as one would expect during compilation. Sadly, there are some downsides:
1. Not all generators yet support compilation databases, and projects not using CMake would still be kind of hung-up to dry, although, there exists a tool which may yet ameliorate this: <a href="https://github.com/rizsotto/Bear" style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">bear</a>. Still not exactly clean or ideal, and who wants to add <strong style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">more</strong> dependencies?
2. I think that the biggest generator missing compilation databases is the native MSVC one, and bear is aimed squarely at *NIX.
3. Added complexity from the compilation database API itself: an individual source file can return multiple potential configurations depending on differences in compilation options in different targets.
4. I'm not sure if this breaks certain of the recent performance enhancements made to the IADM, and/or possibly exacerbates the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">llvm::opt::Option::matches()</code> hot spot.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Lastly, for "dumb" project managers, we may be forced to fall back on some kind of user-configurable parser database, much of the machinery for which could probably be wheedled in to the IADM seeing that it already supports the notion of per-file imports/defines. In fact, the two approaches, both the "smart" CMake-based approach and the basic, "dumb" approach need not necessarily be mutually exclusive: much as the current IADM machinery seems to now encode some basic information regarding the "quality" of the source of import/define information, this approach could probably be extended to do the same for parsing options and override or ignore information as necessary. Since I have no idea as to the scope of other major kdev-clang wishes, I'm not sure how this proposal might fit in with the overall vision for the plugin, but I think there's some serious merit to setting this issue apart from other, configurable functionality as this issue has a direct impact on usability and code correctness.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I apologize in advance if this was longer or wordier than it should have been, or if it was inappropriate to discuss such a drastically different approach in this particular review.</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">@ Craig: For CMake yes, this would work. We actually use that already to find defines and include paths. But no other build tool I know of has this feature. So we still need a generic solution for that. And we cannot use the CompilationDatabase API from clang because of that :( Bear is certainly interesting, but won't work on Windows (not sure about Windows) (b/c it's using LD_PRELOAD).</p></pre>
<br />
<p>- Milian</p>
<br />
<p>On January 19th, 2015, 10:38 a.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 Jan. 19, 2015, 10:38 a.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/compilerswidget.ui <span style="color: grey">(0c90cee)</span></li>
<li>languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.cpp <span style="color: grey">(5b34e75)</span></li>
<li>languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.ui <span style="color: grey">(06a6c51)</span></li>
<li>languages/plugins/custom-definesandincludes/compilerprovider/msvccompiler.cpp <span style="color: grey">(fcd9db7)</span></li>
<li>languages/plugins/custom-definesandincludes/compilerprovider/settingsmanager.cpp <span style="color: grey">(229f456)</span></li>
<li>languages/plugins/custom-definesandincludes/kcm_widget/compilerswidget.cpp <span style="color: grey">(aebbf5a)</span></li>
<li>languages/plugins/custom-definesandincludes/compilerprovider/compilerfactories.h <span style="color: grey">(00042fb)</span></li>
<li>languages/plugins/custom-definesandincludes/compilerprovider/compilerfactories.cpp <span style="color: grey">(8d47690)</span></li>
<li>languages/plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp <span style="color: grey">(24d711d)</span></li>
<li>languages/plugins/custom-definesandincludes/compilerprovider/gcclikecompiler.h <span style="color: grey">(079b78d)</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/icompilerfactory.h <span style="color: grey">(3849bcb)</span></li>
<li>languages/plugins/custom-definesandincludes/compilerprovider/msvccompiler.h <span style="color: grey">(a59a6b7)</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>