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

Mon Jun 22 19:54:44 UTC 2015

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

hey Maciej, there are still some issues with this code. Also, please create separate review requests instead of adding yet more reviewable stuff to this one, or we will never get a patch included. Can we fix the stuff first and then continue with new code?

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

    instead of duplicating the list, push it into a variable and conditoinally add the kdevclangrefactor lib into the list variable.

clangsupport.cpp (line 37)
<https://git.reviewboard.kde.org/r/123963/#comment55986>

    here and elsewhere: Do not use upper cases / camelCasing for filenames. make them all-lowercase.

    (is this new - or did I simply never notice this before?)

clangsupport.cpp (line 232)
<https://git.reviewboard.kde.org/r/123963/#comment55985>

    missing a compile guard. in general - is there any way to write this code _without_ the compile guard?

refactoring/CMakeLists.txt (line 5)
<https://git.reviewboard.kde.org/r/123963/#comment55988>

    as I said above - all of these should be lowercase

refactoring/CMakeLists.txt (line 16)
<https://git.reviewboard.kde.org/r/123963/#comment55989>

    ? we already use clang for language support

refactoring/DocumentCache.cpp (line 26)
<https://git.reviewboard.kde.org/r/123963/#comment55990>

    style is still wrong. please put the opening braces of function and class contexts into their own line

        void foo()
        {
            ....
        }

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

    take sourceFile and position by const&

refactoring/Refactoring.h (line 28)
<https://git.reviewboard.kde.org/r/123963/#comment55993>

    style: #include <QUrl> etc. - without the QtCore prefix

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

    and it should not be handed in as non-const reference.

refactoring/RefactoringInfo.h (line 40)
<https://git.reviewboard.kde.org/r/123963/#comment55992>

    style: space around operators

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

    a "Refactoring" namespace might be a good idea here, esp. for the free functions below.

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

    const& for url and cursor

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

    take QUrl and Cursor by const&

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

    maybe just `using namespace llvm;`?

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

    buildPath should be taken by const&

he

- Milian Wolff

On June 20, 2015, 9:26 p.m., Maciej Poleski wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123963/
> -----------------------------------------------------------
> 
> (Updated June 20, 2015, 9:26 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/NoopRefactoring.h PRE-CREATION 
>   refactoring/NoopRefactoring.cpp PRE-CREATION 
>   refactoring/Refactoring.h PRE-CREATION 
>   refactoring/Refactoring.cpp PRE-CREATION 
>   refactoring/RefactoringInfo.h PRE-CREATION 
>   refactoring/RefactoringInfo.cpp PRE-CREATION 
>   refactoring/RefactoringsGlue.h PRE-CREATION 
>   refactoring/RefactoringsGlue.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/20150622/1ecfed0c/attachment-0001.html>