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

[Milian Wolff]

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


yes, much better - thanks Sergey. There are some parts that I find a bit odd, but overall this is going into the right direction. I esp. like the ability to choose a standard on a per-compiler basis. Good job!


languages/plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp
<https://git.reviewboard.kde.org/r/121777/#comment51061>

    I find this code relativley confusing. creating a compiler just to check its standard afterwards. Rather, I think the supported standards and compiler creation logic should go into the factory and here we just do something like
    
        if (findExecutable("gcc")) {
            GccFactory::registerCompilers(this);
        }
        if (findExecutable("clang")) {
            ClangFactory::registerCompilers(this);
        }
        ...



languages/plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp
<https://git.reviewboard.kde.org/r/121777/#comment51062>

    this is unrelated to this patch no? and does this really still occur? there cannot be a ProjectBaseItem without either projectAboutToBeOpened or projectOpened signals, no?



languages/plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp
<https://git.reviewboard.kde.org/r/121777/#comment51063>

    this is legacy support? remove it, we never released kdevelop 5 yet so we don't need this, or?



languages/plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp
<https://git.reviewboard.kde.org/r/121777/#comment51064>

    the whitespaces are not needed, qDebug adds those automatically



languages/plugins/custom-definesandincludes/compilerprovider/gcclikecompiler.h
<https://git.reviewboard.kde.org/r/121777/#comment51067>

    remove here, this is static data



languages/plugins/custom-definesandincludes/compilerprovider/gcclikecompiler.cpp
<https://git.reviewboard.kde.org/r/121777/#comment51066>

    Use CPP or Cpp instead



languages/plugins/custom-definesandincludes/compilerprovider/gcclikecompiler.cpp
<https://git.reviewboard.kde.org/r/121777/#comment51068>

    I suggest adding a compilerStandards function here, which creates the _QHash_ statically internally and returns that.



languages/plugins/custom-definesandincludes/compilerprovider/gcclikecompiler.cpp
<https://git.reviewboard.kde.org/r/121777/#comment51069>

    I propose you simplify this by doing
    
        const auto standard = stringToCompilerStandard(langaugeStandard());
        proc.start( path(), {languageOption(standard), standardOption(standard), "-dM", "-E", NULL_DEVICE});
    
    the m_languageStandards is static after all (see above).



languages/plugins/custom-definesandincludes/compilerprovider/gcclikecompiler.cpp
<https://git.reviewboard.kde.org/r/121777/#comment51070>

    see above



languages/plugins/custom-definesandincludes/compilerprovider/gcclikecompiler.cpp
<https://git.reviewboard.kde.org/r/121777/#comment51071>

    see above, put this into a free function and make it static



languages/plugins/custom-definesandincludes/compilerprovider/gcclikecompiler.cpp
<https://git.reviewboard.kde.org/r/121777/#comment51072>

    this todo should go to the free function that creates the languageStandards hash map.



languages/plugins/custom-definesandincludes/compilerprovider/icompiler.h
<https://git.reviewboard.kde.org/r/121777/#comment51076>

    please add apidox



languages/plugins/custom-definesandincludes/compilerprovider/icompiler.h
<https://git.reviewboard.kde.org/r/121777/#comment51075>

    make this pure virtual



languages/plugins/custom-definesandincludes/compilerprovider/icompiler.h
<https://git.reviewboard.kde.org/r/121777/#comment51074>

    btw: it's odd to have an interface that implements functions. in the future, this should be cleaned up


- Milian Wolff


On Jan. 6, 2015, 10:49 a.m., Sergey Kalinichev wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121777/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2015, 10:49 a.m.)
> 
> 
> 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/compilerswidget.ui 0c90cee 
>   languages/plugins/custom-definesandincludes/kcm_widget/definesandincludesconfigpage.cpp 75f224d 
>   languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.cpp 5b34e75 
>   languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.ui 3e413b3 
>   languages/plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp 3643e78 
>   languages/plugins/custom-definesandincludes/compilerprovider/gcclikecompiler.h 079b78d 
>   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/msvccompiler.h a59a6b7 
>   languages/plugins/custom-definesandincludes/compilerprovider/msvccompiler.cpp fcd9db7 
>   languages/plugins/custom-definesandincludes/compilerprovider/settingsmanager.cpp 229f456 
>   languages/plugins/custom-definesandincludes/kcm_widget/compilerswidget.cpp aebbf5a 
> 
> 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/20150107/45c1d908/attachment-0001.html>