Review Request 124139: Added clang settings pages

Milian Wolff mail at milianw.de
Thu Jul 2 19:52:02 UTC 2015 


> On June 22, 2015, 7:41 p.m., Milian Wolff wrote:
> > 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?
> 
> Sergey Kalinichev wrote:
>     Hi Milian, thanks for the feedback.
>     
>     2. Maybe "Parser settings", then?
>     3. Yes we can, but that config page called "Custom Defines and Includes". Adding parser arguments there will be a bit confusing imo.
>     So I'm actually thinking about moving all that logic regarding language standards to kdev-clang, and exposing the CompilerProvider through DaIM - to set language standard to compilers.
>     5. Most of the time you change standard (e.g. the project you work on isn't c++11). We could use just the line edit to do it, but not everyone knows how to set the standard there manually, also it's easy to misspell.
>     Actually I'm thinking about adding default and advanced modes. Where the combobox is visible in the default mode, and the lineedit in the advanced mode.
>     7. More stuff can be added later on...
>     8. You mean something like adding tabs dynamically to the "Language Support" config page based on loaded language plugins?

2. Maybe just "C/C++ parser"
5. or just have a combobox for the standard and a lineedit for everything else, except the standard 
7. but currently there is nothing there and it looks very bad.
8. yes something like that


> On June 22, 2015, 7:41 p.m., Milian Wolff wrote:
> > clangsettings/clangsettingsmanager.cpp, line 196
> > <https://git.reviewboard.kde.org/r/124139/diff/2/?file=380874#file380874line196>
> >
> >     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 :-/
> 
> Sergey Kalinichev wrote:
>     This is called once per parse session. Still I don't get what's wrong with it and what should be added to the TODO note?

it's very inefficient. you first allocate a list and then convert it to a vector.


- Milian


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


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/20150702/5bfd8aa1/attachment.html>

More information about the KDevelop-devel mailing list