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
Sat Jun 6 20:17:58 UTC 2015 


> On June 6, 2015, 2:05 p.m., Milian Wolff wrote:
> > 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.
> 
> Maciej Poleski wrote:
>     hey!
>     
>     I will think again about use of rvalue references with special regard to http://stackoverflow.com/questions/14856344/when-should-stdmove-be-used-on-a-function-return-value/14856553#14856553
>     In fact i'm using C++14... is that ok?

no, please stick to C++11 for now.


> On June 6, 2015, 2:05 p.m., Milian Wolff wrote:
> > refactoring/Cache.cpp, line 28
> > <https://git.reviewboard.kde.org/r/123963/diff/2/?file=379045#file379045line28>
> >
> >     style: join next line
> >     
> >     and why do you take by reference? shouldn't it be const ref? also in the other places
> 
> Maciej Poleski wrote:
>     opening brace in the same line as in `for (...) {`?
>     
>     i cannot move object taken by const reference

yes. and you probably shouldn't move in the first place.


> On June 6, 2015, 2:05 p.m., Milian Wolff wrote:
> > refactoring/interface.h, line 77
> > <https://git.reviewboard.kde.org/r/123963/diff/2/?file=379046#file379046line77>
> >
> >     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.
> 
> Maciej Poleski wrote:
>     I used new lines to improve readability - argument strings are quite long, but can change if this style if prefered. How long line can be? (i assumed 80 columns)
>     
>     Maybe i should use other prefix like "build" or something like this. I wanted to emphasize that this function "makes" returned instance (which is more expensive than simply returning already built object).

create is an often used prefix, i.e. createCompilationDatabase. and regarding line length: not too long. we don't enforce a hard limit, but around 100 chars or so you should naturally want to break a line. But break it in the way I showed you please


> On June 6, 2015, 2:05 p.m., Milian Wolff wrote:
> > refactoring/interface.h, line 78
> > <https://git.reviewboard.kde.org/r/123963/diff/2/?file=379046#file379046line78>
> >
> >     using `std::string` inside an `extern "C"` block? not a good idea.
> 
> Maciej Poleski wrote:
>     I only wanted "C" linkage (elude name mangling). Of course calling this function from C is not possible. But KDevelop is written in C++ - building std::string objects is not a problem.
>     
>     Or is using non-POD argument types as parameters in functions with "C" linkage forbidden?

If you use C++ in an extern "C" block, it means that you cannot use it from any language that expects the "C" ABI. So remove either the `extern "C"` (which I'd recommend you do for now), or use non-C++ types here.


> On June 6, 2015, 2:05 p.m., Milian Wolff wrote:
> > refactoring/utils.cpp, line 96
> > <https://git.reviewboard.kde.org/r/123963/diff/2/?file=379049#file379049line96>
> >
> >     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
> 
> Maciej Poleski wrote:
>     This piece of code would repeat inside of implementation of `toRange` function. It has inherent contracts on state of `toRange` invocation (exclusive permission to modify local variables of `toRange`). It isn't usable outside of this function.

then please at least add comments to explain what its supposed to do. `move` is a very loaded term, esp. compared to `std::move`.


> On June 6, 2015, 2:05 p.m., Milian Wolff wrote:
> > refactoring/utils.cpp, line 201
> > <https://git.reviewboard.kde.org/r/123963/diff/2/?file=379049#file379049line201>
> >
> >     ?
> 
> Maciej Poleski wrote:
>     I noticed that DocumentChangeSet can also carry information about file renaming. libTooling will not do that, but such renaming actions may be reasonable (e.g. when we change name of a class which had name similar to name of file with its definition).

ok, please clarify the comment then. in the future, we may handle this then on top of libTooling.


- Milian


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


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/74b21b18/attachment-0001.html>

More information about the KDevelop-devel mailing list