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

Fri Jun 5 17:50:25 UTC 2015

> On Cze 3, 2015, 6:42 po południu, Milian Wolff wrote:
> > refactoring/interface.h, line 56
> > <https://git.reviewboard.kde.org/r/123963/diff/1/?file=378452#file378452line56>
> >
> >     there is also CXCompilationDataBase.h in clang-c - can that be used instead here?

Sadly, it cannot. I don't see any API to interchange informations between clang-c and libTooling.

> On Cze 3, 2015, 6:42 po południu, Milian Wolff wrote:
> > refactoring/CMakeLists.txt, line 93
> > <https://git.reviewboard.kde.org/r/123963/diff/1/?file=378451#file378451line93>
> >
> >     Is this really required? the libs above, are they shared or static? shared libs come with dependencies, and linking to ones own dependency is sufficient and will pull in all of its dependencies. So I really wonder why you need all of this. Maybe b/c you have only static versions of libtooling around?

Shared libraries are probably (slightly) less problematic. But some implementations come with static versions only (at least gentoo). All of Clang documentation I've seen concerns static libraries, thus I expect them to occur in preference to shared on many distributions.

> On Cze 3, 2015, 6:42 po południu, Milian Wolff wrote:
> > refactoring/interface.cpp, line 22
> > <https://git.reviewboard.kde.org/r/123963/diff/1/?file=378453#file378453line22>
> >
> >     I'd change the API, it should take a path to a compilation database file. it will be the same for code for cmake, qmake, xyz - always a compile_commands.json file in the $buildpath - no? or why is the ProjectKind required? If that file does not exist, return a nullptr.

On libTooling side code which loads compilation database from JSON file appends file name to given path (here: http://clang.llvm.org/doxygen/JSONCompilationDatabase_8cpp_source.html#l00124).
On this point I don't assume this path to be equal for all build systems, thus this function (makeCompilationDatabaseFromCMake) is only for CMake. Other build systems will be handled in separate function (and decisin will be made based on @p kind). Note that this function is not part of API.
ProjectKind is possible way to say: "If that file does not exists, create one". It may also affect approach to changes in project structure.

In general I didn't assumed at this point of time, that possible use of Bear will be outside of scope of refactorings module.

> On Cze 3, 2015, 6:42 po południu, Milian Wolff wrote:
> > refactoring/interface.cpp, line 46
> > <https://git.reviewboard.kde.org/r/123963/diff/1/?file=378453#file378453line46>
> >
> >     dead code - remove? or will this be where you are going to implement things?
> >     
> >     also, do you _really_ want to take cache by rvalue ref? I doubt that. take it by value.

In fact real implementation of this function was removed by me before commit. I wasn't convined by its implementation. Current cache is read-only.

We have two problems with cache:
- In fact strings are already in memory and making further copies have only performance penalty. But as tracking state of memory inside of IDocument(Controller) can be difficult, we agree with some penalties still wanting to minimize them.
- This cache will be "intermediate". Its "content" will be registered in ClangTool (which will take address of these strings, not their copies). Cache may grow (new documents may be opened), but memory locations of already existing objects (strings data) must be preserved. Even when container size needs to be increased.

This function will be rewriting (with possibly small cost) one cache (string -> string mapping) to another (internal form which will guarante preservation of strings memory locations on updates to cache). "cache" created by caller will not be especially useful. I take exclusive ownership of this object because I will destroy it trying to elude memory copying.

> On Cze 3, 2015, 6:42 po południu, Milian Wolff wrote:
> > refactoring/interface.cpp, line 50
> > <https://git.reviewboard.kde.org/r/123963/diff/1/?file=378453#file378453line50>
> >
> >     return by value

The same here

- Maciej

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

On Maj 31, 2015, 1:56 po południu, Maciej Poleski wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123963/
> -----------------------------------------------------------
> 
> (Updated Maj 31, 2015, 1:56 po południu)
> 
> 
> 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/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/20150605/dce4c5ec/attachment-0001.html>