Review Request 123963: First bits of refactorings. Skeleton of interface for KDevelop. Build system. CompilationDatabase for CMake projects and ClangTool.

Sat Jun 6 14:05:46 UTC 2015

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

hey there! I have some issues with your excessive use of new C++11 features, I think they are often not idiomatic. please clean this code up a bit and strive for easy-to-read, documented code. while you are of course free to use new C++11 code, it is not meant as a playground where you should use every new feature just because its new.

CMakeLists.txt (line 33)
<https://git.reviewboard.kde.org/r/123963/#comment55678>

    also disable it on anything but linux for now - linking against ncurses won't work on windows e.g.

refactoring/Cache.h (line 25)
<https://git.reviewboard.kde.org/r/123963/#comment55686>

    these comments are not really necessary. if you want to keep them in - ok, but no other file in our code base follows this style

refactoring/Cache.h (line 37)
<https://git.reviewboard.kde.org/r/123963/#comment55679>

    rename it to DocumentsCache - just cache is too generic

refactoring/Cache.cpp (line 28)
<https://git.reviewboard.kde.org/r/123963/#comment55680>

    style: join next line

    and why do you take by reference? shouldn't it be const ref? also in the other places

refactoring/interface.h (line 36)
<https://git.reviewboard.kde.org/r/123963/#comment55683>

    yes. as Kevin and me suggested on the ML - keep it simple and don't care too much about these issues for now. Just concentrate on getting refactoring features in. I.e. feel free to remove this file for now and we can handle this later on.

refactoring/interface.h (line 74)
<https://git.reviewboard.kde.org/r/123963/#comment55684>

    do not use out-parameters. return a struct containing the database and an error message

    or, looking at the overdesigned method itself, I'd still suggest you only pass in a build path. If no database is returned (i.e. nullptr), we can construct an error message on the other side if necessary.

        CompilationDatabase compilationDatabase(const std::string& buildPath);

refactoring/interface.h (line 77)
<https://git.reviewboard.kde.org/r/123963/#comment55682>

    here and below, style:

        ret_t function(arg1_t arg1, arg2_t arg2,
                       arg3_t arg3, ...);

    furthermore, we do not ever call functions `getFoo`, always just `foo`, following the Qt API guidelines.

refactoring/interface.h (line 78)
<https://git.reviewboard.kde.org/r/123963/#comment55681>

    using `std::string` inside an `extern "C"` block? not a good idea.

refactoring/interface.h (line 107)
<https://git.reviewboard.kde.org/r/123963/#comment55685>

    do not take values by rvalue reference. in general, you are misusing std::move in many places. read e.g. http://stackoverflow.com/a/14856553/35250 and maybe even Scott Meyers new book, I'll check up on that next week when I have it in my hands again to cite some stuff.

    But in general: take values by `const&` instead of by `&&` in argument lists. use `std::move` sparingly, where necessary. not blindly all-over the place - it really makes the code ugly for no gain.

refactoring/utils.cpp (line 55)
<https://git.reviewboard.kde.org/r/123963/#comment55687>

    instead of marking stuff as static, wrap it all in an anonymous namespace

refactoring/utils.cpp (line 58)
<https://git.reviewboard.kde.org/r/123963/#comment55688>

    style: join next line, also below.

        ret_t func(...)
        {
            if (...) {
                ...
            } else if (...) {
                ...
            } else {
                ...
            }
            switch (...) {
            case 1:
                ...
                break;
            ...
            }
            ...
        }

refactoring/utils.cpp (line 96)
<https://git.reviewboard.kde.org/r/123963/#comment55692>

    what does this function do? maybe move into a free function instead of using a lambda? it's fancy and new, but not really readable

refactoring/utils.cpp (line 156)
<https://git.reviewboard.kde.org/r/123963/#comment55689>

    style: in .cpp files, please add a `using namespace KDevelop` and remove the `KDevelop::` qualifications.

refactoring/utils.cpp (line 162)
<https://git.reviewboard.kde.org/r/123963/#comment55690>

    ? please translate to english

refactoring/utils.cpp (line 201)
<https://git.reviewboard.kde.org/r/123963/#comment55691>

    ?

- Milian Wolff

On June 6, 2015, 1:31 p.m., Maciej Poleski wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123963/
> -----------------------------------------------------------
> 
> (Updated June 6, 2015, 1:31 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdev-clang
> 
> 
> Description
> -------
> 
> First bits of refactorings.
> Skeleton of interface for KDevelop.
> Build system (really nasty, linked DSO has 18MiB).
> CompilationDatabase for CMake projects and ClangTool (with cache, but without cache updates).
> Interface is not frozen for now.
> 
> Currently building of refactorings is enabled only if Clang 3.6.x is found. In other case build system behaves exactly as current master.
> 
> Refactorings require some contextual informations about projects. I made draft of interface with should feature:
>  - Stable ABI - dlopen (or equivalent) a DSO and if succeed (binary is still compatible with Clang libraries ABI), dlsym (or equivalent) some functions which are to provide all refactorings features
>  - Prototypes (with some implementation) of functions constituting API between KDevelop and refactorings backend
>  - It may be considered to use Qt plugin system to provide implementation of this (or equivalent) interface. This would be more expensive, but also more portable
>  - In worst case this library (static version) can be used with additional driver to make stand-alone executable providing all refactorings features (but it would require additional marshaling and hamper cooperation with user (how to provide UI in such a case?))
> 
> Handling dependencies between Clang/LLVM libraries is a nightmare. I extended FindClang.cmake to find much more libraries and used them to link with first pieces of my code. This is huge, requires additional libraries like zlib and even ordering of dependencies can cause linker errors.
> 
> Interface is not finished
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 875172a8407f4bd9faf330f032a280fa66c2b16f 
>   cmake/FindClang.cmake 6c9bd6ef0242319122dcc7e6fd6cea8d9f0cbfbb 
>   refactoring/CMakeLists.txt PRE-CREATION 
>   refactoring/Cache.h PRE-CREATION 
>   refactoring/Cache.cpp PRE-CREATION 
>   refactoring/interface.h PRE-CREATION 
>   refactoring/interface.cpp PRE-CREATION 
>   refactoring/utils.h PRE-CREATION 
>   refactoring/utils.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/123963/diff/
> 
> 
> Testing
> -------
> 
> Compiles on my Gentoo system. It makes sense to build this only on system with Clang 3.6.
> 
> 
> Thanks,
> 
> Maciej Poleski
> 
>

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