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

Milian Wolff mail at milianw.de
Thu Jul 2 13:39:34 UTC 2015 

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



refactoring/documentcache.h (line 56)
<https://git.reviewboard.kde.org/r/123963/#comment56320>

    don't pass in the doc controller



refactoring/documentcache.h (line 59)
<https://git.reviewboard.kde.org/r/123963/#comment56317>

    remove get prefix



refactoring/documentcache.h (line 61)
<https://git.reviewboard.kde.org/r/123963/#comment56316>

    make the function const



refactoring/documentcache.h (line 68)
<https://git.reviewboard.kde.org/r/123963/#comment56319>

    don't store it, use `ICore::self()->documentController()` where required



refactoring/documentcache.h (line 70)
<https://git.reviewboard.kde.org/r/123963/#comment56318>

    space after comma



refactoring/documentcache.cpp (line 46)
<https://git.reviewboard.kde.org/r/123963/#comment56324>

    `const auto ctx = ...`
    
    or move the const to the beginning



refactoring/documentcache.cpp (line 61)
<https://git.reviewboard.kde.org/r/123963/#comment56321>

    I'd prefer you return a ptr instead of a reference, this style is more common in the Qt world



refactoring/documentcache.cpp (line 91)
<https://git.reviewboard.kde.org/r/123963/#comment56323>

    missing third parameter "this" to disconnect the lambda automatically when the DocumentCache gets destroyed



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

    don't pass the controller
    
    in general, this just shows again how overdesigned this code is. you couple against KTextEditor, Qt and KDevelop API, but write it in a C API... I hope you'll clean this up in the future (in a separate patch review).



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

    here and below: const before typename



refactoring/interface.cpp (line 92)
<https://git.reviewboard.kde.org/r/123963/#comment56327>

    here and below: const before typename



refactoring/kdevrefactorings.cpp (line 48)
<https://git.reviewboard.kde.org/r/123963/#comment56329>

    style:
    
        Foo::Foo()
            : Bar(...)
        {
            ...
        }



refactoring/kdevrefactorings.cpp (line 56)
<https://git.reviewboard.kde.org/r/123963/#comment56330>

    join with next line



refactoring/kdevrefactorings.cpp (line 107)
<https://git.reviewboard.kde.org/r/123963/#comment56331>

    qDebug adds spaces automatically, not required here and elswhere you use qCDebug/clangDebug



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

    this is wrong, remove it, *and* remove the QObject as base class above.



refactoring/kdevrefactorings_disabled.h (line 44)
<https://git.reviewboard.kde.org/r/123963/#comment56333>

    ? just remove it all


- Milian Wolff


On July 1, 2015, 9:42 p.m., Maciej Poleski wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123963/
> -----------------------------------------------------------
> 
> (Updated July 1, 2015, 9:42 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 
>   clangsupport.h 8ed1ec90bbbc41d7c7a94d926e0951c729a6194c 
>   clangsupport.cpp e22c55426a2e839ec11cbe0b2fe1e13721b0583a 
>   cmake/FindClang.cmake 6c9bd6ef0242319122dcc7e6fd6cea8d9f0cbfbb 
>   refactoring/CMakeLists.txt PRE-CREATION 
>   refactoring/documentcache.h PRE-CREATION 
>   refactoring/documentcache.cpp PRE-CREATION 
>   refactoring/interface.h PRE-CREATION 
>   refactoring/interface.cpp PRE-CREATION 
>   refactoring/kdevrefactorings.h PRE-CREATION 
>   refactoring/kdevrefactorings.cpp PRE-CREATION 
>   refactoring/kdevrefactorings_disabled.h PRE-CREATION 
>   refactoring/nooprefactoring.h PRE-CREATION 
>   refactoring/nooprefactoring.cpp PRE-CREATION 
>   refactoring/refactoring.h PRE-CREATION 
>   refactoring/refactoring.cpp PRE-CREATION 
>   refactoring/refactoringcontext.h PRE-CREATION 
>   refactoring/refactoringcontext.cpp PRE-CREATION 
>   refactoring/refactoringinfo.h PRE-CREATION 
>   refactoring/refactoringinfo.cpp PRE-CREATION 
>   refactoring/renamevardeclrefactoring.h PRE-CREATION 
>   refactoring/renamevardeclrefactoring.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/20150702/00830363/attachment-0001.html>

More information about the KDevelop-devel mailing list