Review Request 124139: Added clang settings pages

Milian Wolff mail at milianw.de
Mon Jun 22 19:41:11 UTC 2015 

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


Hey Sergey. First of all - nice! I've recently seen that new Qt code wants `-fPIC` or `-fPIE` and otherwise reports a `#error` which we then add as a problem to every translation unit - this patch will help here!

The code review below shows some minor nitpicks. I'm not giving you a ship-it yet though because I really would like to see some improvements on the UI part:

1. project settings
   * I dislike that it says "Clang" here - yes we do use it, but no this is not the compiler and people will mix it up. This is just about our Clang language backend, not about what we will pass to the compiler.
   * Also, this duplicates some stuff with the custom includes/defines, no? Can't we add custom arguments there already? Why can this not be reused?
   * The dot as a root project path is not nice, I think. Could you maybe unalias relative paths there?
   * the standard is given in the combobox _and_ repeated in the lineedit below - why?
2. session settings
   * the tab with only one check box is a waste of space.
   * could we maybe have a "per-language" section in the "Language Support" instead of adding another settings page?


clangsettings/clangsettingsmanager.h (line 38)
<https://git.reviewboard.kde.org/r/124139/#comment55972>

    { on new line, like below



clangsettings/clangsettingsmanager.h (line 79)
<https://git.reviewboard.kde.org/r/124139/#comment55973>

    please return a QVector and mark ParserSettingsEntry as Q_MOVABLE_TYPE with Q_DECLARE_TYPEINFO



clangsettings/clangsettingsmanager.cpp (line 186)
<https://git.reviewboard.kde.org/r/124139/#comment55974>

    QStringLiteral



clangsettings/clangsettingsmanager.cpp (line 196)
<https://git.reviewboard.kde.org/r/124139/#comment55975>

    juk, this is ugly but OK for now as it's (hopefully) not called often (or is it, once per file?). Anyhow, add a TODO note to improve this in the future. I hate it, that there is no simple API to split into a QVector :-/



clangsettings/configwidget.h (line 52)
<https://git.reviewboard.kde.org/r/124139/#comment55976>

    please use "signals" and "slots" below



clangsettings/configwidget.h (line 63)
<https://git.reviewboard.kde.org/r/124139/#comment55977>

    QScopedPointer and declare a dtor with a `= default` definition in the .cpp file. You are currently leaking here.



clangsettings/configwidget.cpp (line 79)
<https://git.reviewboard.kde.org/r/124139/#comment55978>

    QStringLiteral



clangsettings/pathsmodel.h (line 41)
<https://git.reviewboard.kde.org/r/124139/#comment55981>

    QVector (and make sure its Q_MOVABLE_TYPE)



clangsettings/pathsmodel.h (line 52)
<https://git.reviewboard.kde.org/r/124139/#comment55979>

    style:
    
        signals:
            void changed();



clangsettings/pathsmodel.h (line 57)
<https://git.reviewboard.kde.org/r/124139/#comment55980>

    QVector



clangsettings/sessionsettings/sessionsettings.h (line 49)
<https://git.reviewboard.kde.org/r/124139/#comment55983>

    QScopedPointer. leaked again



clangsettings/sessionsettings/sessionsettings.ui (line 36)
<https://git.reviewboard.kde.org/r/124139/#comment55982>

    If disabled, macros will not be included in the code-completion results. This can improve the responsiveness of code-completion in some cases.



duchain/parsesession.cpp (line 58)
<https://git.reviewboard.kde.org/r/124139/#comment55984>

    use QByteArrayLiteral now, also below and above


- Milian Wolff


On June 21, 2015, 6:37 p.m., Sergey Kalinichev wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124139/
> -----------------------------------------------------------
> 
> (Updated June 21, 2015, 6:37 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdev-clang
> 
> 
> Description
> -------
> 
> This adds two pages: One for session settings (code-completion, assistants), another one for project settings (parser command-line arguments).
> 
> Known issue: now we can set language standard in two places: DaIM - for deducing standard macros/includes and here - for the internal parser.
> To solve this problem we can copy/move compilers infrastructure to kdev-clang. Also we can expose the compiler provider interface through DaIM (which is a much better solution imo). Suggestions?
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 875172a 
>   clangparsejob.cpp bea4fc0 
>   clangsettings/CMakeLists.txt PRE-CREATION 
>   clangsettings/clangconfigpage.h PRE-CREATION 
>   clangsettings/clangconfigpage.cpp PRE-CREATION 
>   clangsettings/clangprojectconfig.kcfg PRE-CREATION 
>   clangsettings/clangprojectconfig.kcfgc PRE-CREATION 
>   clangsettings/clangsettingsmanager.h PRE-CREATION 
>   clangsettings/clangsettingsmanager.cpp PRE-CREATION 
>   clangsettings/clangsettingsplugin.h PRE-CREATION 
>   clangsettings/clangsettingsplugin.cpp PRE-CREATION 
>   clangsettings/configwidget.h PRE-CREATION 
>   clangsettings/configwidget.cpp PRE-CREATION 
>   clangsettings/configwidget.ui PRE-CREATION 
>   clangsettings/kdevclangsettings.json PRE-CREATION 
>   clangsettings/pathsmodel.h PRE-CREATION 
>   clangsettings/pathsmodel.cpp PRE-CREATION 
>   clangsettings/sessionsettings/sessionconfig.kcfg PRE-CREATION 
>   clangsettings/sessionsettings/sessionconfig.kcfgc PRE-CREATION 
>   clangsettings/sessionsettings/sessionconfigskeleton.h PRE-CREATION 
>   clangsettings/sessionsettings/sessionsettings.h PRE-CREATION 
>   clangsettings/sessionsettings/sessionsettings.cpp PRE-CREATION 
>   clangsettings/sessionsettings/sessionsettings.ui PRE-CREATION 
>   codecompletion/context.cpp f26921f 
>   codegen/adaptsignatureassistant.cpp 2cdd34b 
>   duchain/CMakeLists.txt e07ef70 
>   duchain/clangparsingenvironment.h 2e4ea8b 
>   duchain/clangparsingenvironment.cpp 20a2dbf 
>   duchain/parsesession.h 74999de 
>   duchain/parsesession.cpp 563d227 
>   duchain/unknowndeclarationproblem.cpp 908a518 
> 
> Diff: https://git.reviewboard.kde.org/r/124139/diff/
> 
> 
> Testing
> -------
> 
> 
> File Attachments
> ----------------
> 
> project settings.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/06/21/f4bb2220-dc63-478d-b423-f31434496afb__project_settings.png
> session settings.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/06/21/e1f59a46-f0b4-46fa-9f19-ffca4b887cf5__session_settings.png
> 
> 
> Thanks,
> 
> Sergey Kalinichev
> 
>

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

More information about the KDevelop-devel mailing list