irony-cdb-json/irony-server: Caches last used compilation database #499
Conversation
server/src/Irony.cpp
Outdated
| clang_CompilationDatabase_fromDirectory(buildDir.c_str(), &error); | ||
| bool dbLoaded; | ||
| CXCompilationDatabase db; | ||
| std::tie(dbLoaded, db) = cdbCache_.getCompilationDatabase(buildDir); |
There was a problem hiding this comment.
I don't know how better it is, but I feel like the caching could have the same API as the Clang API.
That is, it could have a signature such as:
CXCompilationDatabase
CompDBCache::fromDirectory(const char *BuildDir,
CXCompilationDatabase_Error *ErrorCode);So the caching is just decoration/optimisation that can be used as a drop-in replacement.
The cache only deals with caching and not with writing S-Expr results.
server/src/Irony.h
Outdated
| @@ -133,6 +134,20 @@ class Irony { | |||
| void computeCxUnsaved(); | |||
|
|
|||
| private: | |||
| class CDBCache { | |||
There was a problem hiding this comment.
I'm not very fond of inner classes, I would prefer a top-level CompDB[Cache] class.
The class could mirror the libclang naming:
- getCompilationDatabase could be renamed
fromDirectory.
server/src/Irony.h
Outdated
| @@ -124,7 +125,7 @@ class Irony { | |||
| /// \endcode | |||
| /// | |||
| void getCompileOptions(const std::string &buildDir, | |||
| const std::string &file) const; | |||
| const std::string &file); | |||
There was a problem hiding this comment.
I don't think I would drop the const here, instead I would make the cache variable mutable, caching is a valid use case for mutable IMHO.
server/src/Irony.cpp
Outdated
| clear(); | ||
| } | ||
|
|
||
| std::tuple<bool, CXCompilationDatabase> Irony::CDBCache::getCompilationDatabase(const std::string &buildDir) { |
There was a problem hiding this comment.
CompilationDatabase does not necessarily imply JSONCompilationDatabase.
Clang abstracts a CompilationDatabase, which can be a JSONCompilationDatabase, but it can also be something else.
So I would reorganize the code so that the logic is like this:
- stat
$build_dir/compile_commands.json - if stat worked
- if it matches what's the cache variables (path, mtime, ...), return the cache result
- else, call clang_fromDirectory, cache the result and return it
- else, if stat did not work, maybe it's another kind of compilation database, just return the result from clang_fromDirectory, no caching
Using this logic does not hinder the normal behavior of libclang, but if we found a compile_commands.json, we are faster.
itollefsen
force-pushed
the
cache-last-used-cdb
branch
from
e82c735
to
e1e5530
Compare
|
I read the feedback and then forgot all about it. It's been a busy winter. Anyway, I've now tried to incorporate the changes you requested. Including splitting the cache class from the Irony class and putting it in its own translation unit(s). As for other formats - yes, I had completely forgotten about them. Now remedied. Although I think it would be a good (future) improvement to add support for caching more formats (it currently only caches results for JSON compilation databases, as you noted). |
There was a problem hiding this comment.
Thanks for the update!
I suggested a few changes which I believe could improve readability, but it looks good already, so thank you!
As for other formats - yes, I had completely forgotten about them. Now remedied. Although I think it would be a good (future) improvement to add support for caching more formats (it currently only caches results for JSON compilation databases, as you noted).
I know only about one, a compile_flags.txt similar to the .clang_complete irony mode already supports:
I don't think caching this file is relevant.
itollefsen
force-pushed
the
cache-last-used-cdb
branch
from
e1e5530
to
cc4364f
Compare
A compilation database can be very large and take a long time to read and parse, which happens quite frequently (when opening a file for instance). This caches the last read/parsed compilation database based on its filename and last modification time.
itollefsen
force-pushed
the
cache-last-used-cdb
branch
from
cc4364f
to
2e95654
Compare
|
Thank you for your patience and for the change itself, which is quite useful! |
Release 1.4.0 In this release: - cache compilation database [SarcasmGH-499] - add support for new libclang versions, which should now support newer versions automatically - CMake modernization, require CMake >= 3 - improved compile options adjustement [SarcasmGH-542]
A compilation database can be very large and take a long time to read
and parse, which happens quite frequently (when opening a file for
instance).
This caches the last read/parsed compilation database based on its
filename and last modification time.
This could be extended to cache N number of databases instead of just
one, which would be useful when working on multiple projects
simultaneously.