<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/104930/">http://git.reviewboard.kde.org/r/104930/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 13th, 2012, 2:57 p.m., <b>David Nolden</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;">I don't like the "language features" title too much, I think it's too vague and confusable. Wouldn't it be better to call it "language version"? Then, this could be added more generally into the framework, as it's useful for most languages. For example python will need it to distinguish between python-2 and python-3. I know that C is not an older version of C++, but this is how we model it in our language support.
Also, setting a project-global flag is not really scalable. For example, a project may contain both C and C++ code if it has some library copied into some sub-folder.
Therefore, I think the project-controller should be asked for the language, maybe like this: "QString version = projectController->languageVersionForUrl( url, ILanguage* language )". ILanguage again could supply a list of language-versions, which the project-manager would need to show somewhere in the UI to let the user pick, or could guess automatically.</pre>
</blockquote>
<p>On May 14th, 2012, 8:51 a.m., <b>Alexandre Courbot</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 David,
Working with "features" over "versions" makes more sense actually, because some of the features may apply to different versions of the languages (for instance, compiler flags that enable some construct that works with both C and C++). If we work with version, we are restraining ourselves to a linear progression of features, which is quite limiting. LLVM/CLang works with something similar to these "features", and that's for a good reason.
I totally agree that using the project controller to get the list of supported features is the best idea. My project-global flag was mostly for testing purposes, I will try and adapt it to use the controller instead. But I really do think we should keep the features as they are.</pre>
</blockquote>
<p>On May 23rd, 2012, 7:48 p.m., <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;">I'm with Alexandre here - flags are simple to check and implement and by far more powerful. But:
The issue will lie in creating a useable interface/generalization for these actions such that they are useful for other plugins... people might "enable" different features for C++ while for other languages they can only pick between different versions (you cannot be both, python 2 and 3).</pre>
</blockquote>
<p>On May 25th, 2012, 5:48 a.m., <b>Alexandre Courbot</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;">Does it need to be generalized to other plugins? Right now the implementation is 100% internal to the C++ plugin and it looks fine this way. Actually I would not even know which interface class should implement this generalization.</pre>
</blockquote>
<p>On May 28th, 2012, 5:28 p.m., <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;">Personally I wouldn't say it's too bad if we first get a proper solution for c++ and then think about generalization if we find another language where it would be helpful. Not sure if Sven wants to support two different versions of Python, but I'm pretty sure that I don't want to keep backwards compatibility in PHP.</pre>
</blockquote>
<p>On May 28th, 2012, 6:29 p.m., <b>Sven Brauch</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;">I don't really want to do that, and if, then through two mutually exclusive versions of the plugin.</pre>
</blockquote>
<p>On May 29th, 2012, 1:12 a.m., <b>Alexandre Courbot</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;">Ok, good - then I think I will go for a C++ implementation only first. Other languages might want to do that differently anyway. And if we want to generalize later it is just a matter of mapping interfaces and should not be too big a change.</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;">Finally got some more time to give it a thought - here is what I intend to do, feedback is welcome.
Language features should be something totally abstract, as they could litterally mean "features" (like C99 or C++11 support for C) or "versions" (like Python 2 and 3). They are needed by ILanguageSupport::createParseJob as the parser will want to know how to parse the file, but can be specified in two different places:
1) The language support could give a hint according to the url of the file to parse (for instance, ".c" extensions should be parsed as C99 files and not as C++11).
2) The project builder could have even more insight as to how a file should be interpreted. For instance, if it knows that a given url will be compiled using a C++ compiler, it can disable support for C++11. Same thing applies for C.
So the implementation would be through two languageFeatures() methods in IBuildSystemManager and ILanguageSupport, which return a QString representing the features to enable. This string is obtained by the background parser (or any other invoker of ILanguageSupport::createParseJob()), which first tries to obtain it from the build manager (if available) and falls back to language support if nothing interesting came from the build manager. It then passes the string as an additional argument of createParseJob(). That way the language support can tune its parser to correctly parse the file, depending on how the build system thinks the file should be parsed (or at worse, depending on how the file url looks).
How does that sound? Implementing this would require just a few changes to KDevplatform, and should scale gracefully to languages other than C/C++.
</pre>
<br />
<p>- Alexandre</p>
<br />
<p>On May 13th, 2012, 6:41 a.m., Alexandre Courbot wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDevelop.</div>
<div>By Alexandre Courbot.</div>
<p style="color: grey;"><i>Updated May 13, 2012, 6:41 a.m.</i></p>
<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;"> Currently the parser is a pure C++ parser with C++11 elements, without
any option to disable C++11 or to make it more C compatible. C
compatibility in particular is a big challenge since many C++ tokens can
be used as identifiers in C, and C also has some constructs that are not
valid in C++ (like designated initializers or casted braced init lists).
This patch adds "language features" which are flags that enable specific
language support in the parser. Currently flags exist for C99, C++ and
C++11. Code that invokes the parser can use the new
setLanguageFeatures() method to change the current set of supported
features (the default is C++ | C++11). This is useful for instance for
C-specific projects.
CppLanguageSupport::createParseJob has been modified to check for a
"Language" entry in the configuration of the project the parsed file
belongs to. Project controllers can set this option if they know that
the project is limited to C, C++ or C++11 files. Other means (for
instance, based on file extension of Makefile analysis) could be
implemented in the future.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </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;">Modified kdev-kernel to set that "Language=C" option to the projects it manages. Checked that C++ keywords like "class", "private" were indeed treated as identifiers. This alone improves kernel parsing quality a great deal.
Made sure that parsing of C++ projects is not affected in any way by this patch.</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/cpp/cpplanguagesupport.cpp <span style="color: grey">(4ee1ffe)</span></li>
<li>languages/cpp/cppparsejob.h <span style="color: grey">(debb1b0)</span></li>
<li>languages/cpp/cppparsejob.cpp <span style="color: grey">(1e0a51b)</span></li>
<li>languages/cpp/parser/languagefeatures.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/cpp/parser/lexer.h <span style="color: grey">(3494cbd)</span></li>
<li>languages/cpp/parser/lexer.cpp <span style="color: grey">(81302d7)</span></li>
<li>languages/cpp/parser/parser.h <span style="color: grey">(c519891)</span></li>
<li>languages/cpp/parser/parser.cpp <span style="color: grey">(edeab85)</span></li>
<li>languages/cpp/parser/tests/test_parser.h <span style="color: grey">(dea9902)</span></li>
<li>languages/cpp/parser/tests/test_parser.cpp <span style="color: grey">(804f379)</span></li>
<li>languages/cpp/preprocessjob.cpp <span style="color: grey">(803c237)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/104930/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>