Review Request 121777: Make it possible to change language standard for the parser

[Nicolai Hähnle]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121777/#review72974
-----------------------------------------------------------


This seems like a _very_ useful improvement, but the design rubs me the wrong way in several places, which can be summed up with the following observations:

1. Different compilers support different standards.
2. The standard which is in use is not a property of the _compiler_, but a property of _individual source files_ within a project.

Note how the code design is incompatible with these two observations in a number of important ways:

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 `QList<QString> ICompiler::supportedStandards() const`.

1b. The list of standards changes; in fact, the list of standards is already out of date, since C++14 is a thing.

2. 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.
 
For example, note how the `GccLikeCompiler::defines()` implementation caches the result once it has been obtained, and subsequent changes to the standard will simply be ignored.

I suggest to eliminate `ICompiler::setStandard()`, and instead pass the standard as a parameter to `ICompiler::defines(const QString& standard)` and `ICompiler::includes(const QString& standard)`.

I see two possible alternatives:

1. Create another class that is layered on top of ICompiler that plays the role of "compiler + standard", or
2. 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.)


languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.cpp
<https://git.reviewboard.kde.org/r/121777/#comment50782>

    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.


- Nicolai Hähnle


On Jan. 1, 2015, 11:10 vorm., Sergey Kalinichev wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121777/
> -----------------------------------------------------------
> 
> (Updated Jan. 1, 2015, 11:10 vorm.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevelop
> 
> 
> Description
> -------
> 
> 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).
> 
> 
> Diffs
> -----
> 
>   languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.ui 3e413b3 
>   languages/plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp 3643e78 
>   languages/plugins/custom-definesandincludes/compilerprovider/gcclikecompiler.cpp bc99d8d 
>   languages/plugins/custom-definesandincludes/compilerprovider/icompiler.h 3627ebd 
>   languages/plugins/custom-definesandincludes/compilerprovider/icompiler.cpp 0bf6a44 
>   languages/plugins/custom-definesandincludes/compilerprovider/settingsmanager.cpp 229f456 
>   languages/plugins/custom-definesandincludes/kcm_widget/definesandincludesconfigpage.cpp 75f224d 
>   languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.h 446ddb5 
>   languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.cpp 5b34e75 
> 
> Diff: https://git.reviewboard.kde.org/r/121777/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sergey Kalinichev
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20150102/e06a2355/attachment-0001.html>