Review Request 118194: Make it possible to select compiler at run-tume to deduce standard includes/defines

Milian Wolff mail at milianw.de
Mon May 26 15:57:14 UTC 2014 

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


there's still some duplicated code. And I do stay at what I said earlier: The interface must stay localizable (esp. the "none"), and the compiler names should be "pretty" ("GCC").

Also, I'd rename DAICompilerProvider to CompilerProvider and the ICompilerProvider etc. pp. stuff to just ICompiler. The IDAICompilerProvider should be removed, I think.


languages/cpp/includepathcomputer.cpp
<https://git.reviewboard.kde.org/r/118194/#comment40694>

    nitpick: add a space after for



languages/cpp/includepathcomputer.cpp
<https://git.reviewboard.kde.org/r/118194/#comment40695>

    nitpick: if (noProject) {



languages/cpp/includepathcomputer.cpp
<https://git.reviewboard.kde.org/r/118194/#comment40696>

    nitpick: for (const auto& dir : ...->includes(nullptr)) {



languages/cpp/includepathcomputer.cpp
<https://git.reviewboard.kde.org/r/118194/#comment40697>

    nitpick: } else if ...



languages/plugins/custom-definesandincludes/compilerprovider/daicompilerprovider.h
<https://git.reviewboard.kde.org/r/118194/#comment40700>

    nitpick: add newline before



languages/plugins/custom-definesandincludes/compilerprovider/daicompilerprovider.cpp
<https://git.reviewboard.kde.org/r/118194/#comment40701>

    I'd refactor this code (and the one below) to read:
    
    {
      auto project = item ? item->project() : nullptr;
      Q_ASSERT(m_projects.contains(project));
      auto info = m_projects[project];
      Q_ASSERT(info.compiler);
      return info.compiler->defines(info.path);
    }
    
    The first four lines can (and should) even be shared by both, ::defines and ::includes functions
      



languages/plugins/custom-definesandincludes/compilerprovider/daicompilerprovider.cpp
<https://git.reviewboard.kde.org/r/118194/#comment40702>

    move this block before the assignment to m_projects[project] and operate directly on the compiler object



languages/plugins/custom-definesandincludes/compilerprovider/daicompilerprovider.cpp
<https://git.reviewboard.kde.org/r/118194/#comment40703>

    as I said before - there must be two things here: a user-visible name of the compiler, potentially translatable. E.g. "GCC" or "MSVC". Then, there should be getter function for the path to the default-executable. This would use "gcc" or "msvc". and KStandardDirs::findExe.



languages/plugins/custom-definesandincludes/compilerprovider/daicompilerprovider.cpp
<https://git.reviewboard.kde.org/r/118194/#comment40704>

    the comparison to "none" is wrong, this must stay localizable. 
    
    I suggest you create a dummy provider for this case which just returns nothing. 



languages/plugins/custom-definesandincludes/compilerprovider/icompilerprovider.h
<https://git.reviewboard.kde.org/r/118194/#comment40705>

    please add documentation to this class



languages/plugins/custom-definesandincludes/compilerprovider/idaicompilerprovider.h
<https://git.reviewboard.kde.org/r/118194/#comment40706>

    This class is only required for the bridge between settings and actual plugin, right?
    
    Can't you just include daicompilerprovider.h and use that directly instead of adding this interface?
    
    Just query for the plugin directly from the settings using its name (kdevcompilerprovider you named it, afaik).



languages/plugins/custom-definesandincludes/definesandincludesmanager.cpp
<https://git.reviewboard.kde.org/r/118194/#comment40708>

    this and b::includes elow share the same code - extract it into a helper function and use that here
    
    



languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.ui
<https://git.reviewboard.kde.org/r/118194/#comment40707>

    must be localizable ("none" in other languages), and pretty-printed (i.e. GCC, MSVC)



languages/plugins/custom-definesandincludes/settingsmanager.h
<https://git.reviewboard.kde.org/r/118194/#comment40698>

    this is wrong, you'll have to create your own export file similar to those we already have. 



languages/plugins/custom-definesandincludes/settingsmanager.cpp
<https://git.reviewboard.kde.org/r/118194/#comment40699>

    "" -> QString()


- Milian Wolff


On May 23, 2014, 12:27 p.m., Sergey Kalinichev wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118194/
> -----------------------------------------------------------
> 
> (Updated May 23, 2014, 12:27 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevelop
> 
> 
> Description
> -------
> 
> To make it all work I had to turn IADM into global plugin, that way it can be used to retrieve includes/defines for files without project.
> Also I made SettingsManager a shared library to ease access to it from different places.
> 
> 
> Diffs
> -----
> 
>   languages/cpp/CMakeLists.txt 377f35e 
>   languages/cpp/cpplanguagesupport.cpp 0f3bd4a 
>   languages/cpp/cpputils.h c951bd3 
>   languages/cpp/cpputils.cpp 380dd4e 
>   languages/cpp/includepathcomputer.cpp 822b0bc 
>   languages/cpp/msvcdefinehelper.cpp 9b131ed 
>   languages/cpp/setuphelpers.h 012dcf4 
>   languages/cpp/setuphelpers.cpp 57505fc 
>   languages/cpp/setuphelpers_gcc_like.cpp b261589 
>   languages/cpp/setuphelpers_msvc.cpp 5576c01 
>   languages/cpp/tests/CMakeLists.txt d6eb67a 
>   languages/cpp/tests/test_cppfiles.cpp 76486d0 
>   languages/plugins/custom-definesandincludes/CMakeLists.txt be7d6e8 
>   languages/plugins/custom-definesandincludes/compilerprovider/CMakeLists.txt PRE-CREATION 
>   languages/plugins/custom-definesandincludes/compilerprovider/daicompilerprovider.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/compilerprovider/daicompilerprovider.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/compilerprovider/gcclikeprovider.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/compilerprovider/gcclikeprovider.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/compilerprovider/icompilerprovider.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/compilerprovider/idaicompilerprovider.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/compilerprovider/kdevcompilerprovider.desktop.cmake PRE-CREATION 
>   languages/plugins/custom-definesandincludes/compilerprovider/msvcdefinehelper.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/compilerprovider/msvcprovider.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/compilerprovider/msvcprovider.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/definesandincludesmanager.h 12a7763 
>   languages/plugins/custom-definesandincludes/definesandincludesmanager.cpp bdce956 
>   languages/plugins/custom-definesandincludes/kcm_widget/CMakeLists.txt 0d8c830 
>   languages/plugins/custom-definesandincludes/kcm_widget/customdefinesandincludes.kcfg 3b49940 
>   languages/plugins/custom-definesandincludes/kcm_widget/customdefinesandincludes.kcfgc 720af41 
>   languages/plugins/custom-definesandincludes/kcm_widget/kcm_customdefinesandincludes.cpp 75f5063 
>   languages/plugins/custom-definesandincludes/kcm_widget/kcm_kdevcustomdefinesandincludes.desktop ef63c57 
>   languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.cpp 1456217 
>   languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.ui 4e512a9 
>   languages/plugins/custom-definesandincludes/kdevdefinesandincludesmanager.desktop.cmake ad08867 
>   languages/plugins/custom-definesandincludes/settingsmanager.h 9ae23d6 
>   languages/plugins/custom-definesandincludes/settingsmanager.cpp dc7aa3e 
>   languages/plugins/custom-definesandincludes/tests/plugintest.cpp 1cfa2e4 
>   projectmanagers/custom-buildsystem/tests/projects/builddirproject/.kdev4/builddirproject.kdev4 33c8568 
>   projectmanagers/custom-buildsystem/tests/projects/multipathproject/.kdev4/multipathproject.kdev4 fd14aca 
>   projectmanagers/custom-buildsystem/tests/projects/simpleproject/.kdev4/simpleproject.kdev4 2c4bac5 
> 
> Diff: https://git.reviewboard.kde.org/r/118194/diff/
> 
> 
> Testing
> -------
> 
> All tests pass (except cppspecialcompletion, which doesn't work for me anyway).
> Also I've tested it with 3 opened projects in one session, works fine for me.
> 
> 
> File Attachments
> ----------------
> 
> kdevplatform.diff
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/05/23/3a7228b1-c22a-4f00-9ab7-3f4d12fc1107__kdevplatform.diff
> selectCompiler_image.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/05/23/4f199bb9-f4d0-4525-98f5-78e3d3e9829f__selectCompiler_image.png
> 
> 
> Thanks,
> 
> Sergey Kalinichev
> 
>

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

More information about the KDevelop-devel mailing list