Review Request 118718: Add settings dialog to add additional compilers.

Milian Wolff mail at milianw.de
Thu Jun 19 09:47:23 UTC 2014 

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


Woha, great work on the compiler support front - much appreciated. Sorry for my long delay in reviewing this.

There are some minor style issues, but also some fundamental design questions to be solved. In general, this looks solid though - good work!


languages/plugins/custom-definesandincludes/compilerprovider/compilerfactories.cpp
<https://git.reviewboard.kde.org/r/118718/#comment42235>

    could you please add a comment here in the form:
    
    // TODO kf5: use QStringLiteral
    
    thanks



languages/plugins/custom-definesandincludes/compilerprovider/compilerprovider.h
<https://git.reviewboard.kde.org/r/118718/#comment42212>

    const& the compilerpointer?



languages/plugins/custom-definesandincludes/compilerprovider/compilerprovider.h
<https://git.reviewboard.kde.org/r/118718/#comment42213>

    same here and below



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

    use a member initializer list please
    
    DummyCompiler(...)
      : m_name(name)
      , ...
    {
    }



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

    brace on new line, also below



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

    ... : QString()



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

    the parens are not required here



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

    that looks odd - is this to generate some default compiler? then please add a new function for that with a proper name. This pattern is used in a few places in this patch and its always strange and I wonder what it should do :)
    
    generally, some more comments would help so that one directly sees what a given loop will do. for this one e.g. something like "reset project compiler to default"
    
    why is the last factory being used, btw? shouldn't the factory of the compiler that is unregistered be used?



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

    please don't inline stuff
    
    also, don't make these functions virtual, you provide the proper implementation for these things after all.
    
    



languages/plugins/custom-definesandincludes/compilerprovider/icompilerfactory.h
<https://git.reviewboard.kde.org/r/118718/#comment42221>

    please add to the documentation, that editable should be set to false for default compilers detected in the path
    



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

    please document on why and when would this could fail?



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

    const&, also below



languages/plugins/custom-definesandincludes/kcm_widget/compilersmodel.h
<https://git.reviewboard.kde.org/r/118718/#comment42227>

    const&



languages/plugins/custom-definesandincludes/kcm_widget/compilersmodel.cpp
<https://git.reviewboard.kde.org/r/118718/#comment42223>

    a good pattern to use in model implementations, is to define an 
    
    enum Columns {
      NameColumn,
      PathColumn,
      NUM_COLUMNS
    }
    
    and use these enums inplace of the magic numbers
    
    this greatly improves readability and makes it much simpler to extend the model



languages/plugins/custom-definesandincludes/kcm_widget/compilersmodel.cpp
<https://git.reviewboard.kde.org/r/118718/#comment42225>

    this is pretty inefficient just to add a single compiler, no? just use beginInsertRow and insert the compiler at the end, should be just as easy and more efficient?



languages/plugins/custom-definesandincludes/kcm_widget/compilerswidget.cpp
<https://git.reviewboard.kde.org/r/118718/#comment42233>

    this should never happen, or?



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

    this and the loop below essentially does a semantic compilerProvider->setCompilers(cw.compilers())
    
    and that's how I would expect it to be called from here.
    



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

    const& the ptr



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

    why do we serialize this? imo, we should not and instead re-detect at runtime whether system compilers still exist or not. maybe their path changes etc. and we would not detect that then.
    
    so only save editable stuff, i.e. compilers the user added manually



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

    the compiler prefix here and below is not required, is it?
    
    [Compilers]
    [Compilers][1]
    name = ...
    path = ...
    type = ...
    [Compilers][2]
    ...
    
    would suffice, imo


- Milian Wolff


On June 13, 2014, 7:37 a.m., Sergey Kalinichev wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118718/
> -----------------------------------------------------------
> 
> (Updated June 13, 2014, 7:37 a.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevelop
> 
> 
> Description
> -------
> 
> Now there are compilers that auto-detected and those that added manually. The latter exist globally and can be used for any project.
> 
> 
> Diffs
> -----
> 
>   languages/plugins/custom-definesandincludes/compilerprovider/CMakeLists.txt 45cccbc 
>   languages/plugins/custom-definesandincludes/compilerprovider/compilerfactories.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/compilerprovider/compilerfactories.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/compilerprovider/compilerprovider.h 8ab52ab 
>   languages/plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp 6483995 
>   languages/plugins/custom-definesandincludes/compilerprovider/gcclikecompiler.h b96df3b 
>   languages/plugins/custom-definesandincludes/compilerprovider/gcclikecompiler.cpp 97262ec 
>   languages/plugins/custom-definesandincludes/compilerprovider/icompiler.h 77ceb75 
>   languages/plugins/custom-definesandincludes/compilerprovider/icompilerfactory.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/compilerprovider/icompilerprovider.h 21bfc05 
>   languages/plugins/custom-definesandincludes/compilerprovider/msvccompiler.h e0ee668 
>   languages/plugins/custom-definesandincludes/compilerprovider/msvccompiler.cpp 1c6b6c5 
>   languages/plugins/custom-definesandincludes/kcm_widget/CMakeLists.txt ea0fe01 
>   languages/plugins/custom-definesandincludes/kcm_widget/compilersmodel.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/compilersmodel.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/compilerswidget.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/compilerswidget.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/compilerswidget.ui PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/customdefinesandincludes.kcfg 9acb2e0 
>   languages/plugins/custom-definesandincludes/kcm_widget/kcm_customdefinesandincludes.cpp c09b00e 
>   languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.h 3618170 
>   languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.cpp c3a0823 
>   languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.ui 9831b94 
>   languages/plugins/custom-definesandincludes/settingsmanager.h f215268 
>   languages/plugins/custom-definesandincludes/settingsmanager.cpp fb7fd0f 
> 
> Diff: https://git.reviewboard.kde.org/r/118718/diff/
> 
> 
> Testing
> -------
> 
> 
> File Attachments
> ----------------
> 
> addCompiler.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/06/13/613c5d8c-6425-42ad-b0e4-1ccc2384a838__addCompiler.png
> 
> 
> Thanks,
> 
> Sergey Kalinichev
> 
>

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

More information about the KDevelop-devel mailing list