ilya-biryukov added inline comments.
================
Comment at: clangd/ClangdLSPServer.h:23
+
+class ClangdLSPServer {
+ class LSPDiagnosticsConsumer;
----------------
klimek wrote:
> I'd have expected something that's called LSP server to work on the LSP
> protocol level (that is, have a server(iostream) equivalent method).
The idea is to have a class that wraps ClangdServer and is called by clangd's
ProtocolHandlers.cpp to do all the work.
I.e. it's a replacement of what used to be called 'ASTManager'. Any suggestions
for a better name? ClangdLSPHandler?
It's seems hard to implement the interface you suggest right away, lots of code
will have to be moved. Happy to do that in further commits, but suggest we
leave this class as is in this commit.
================
Comment at: clangd/ClangdUnit.h:44
+ /// Reparse with new contents
+ void reparse(StringRef Contents);
+
----------------
klimek wrote:
> One question is how we can handle when we want the old one to stick around
> while we do a full reparse (when #includes change), so we still have fast
> code completion.
My idea was to have two ClangdUnits for that and handle that somewhere
else(i.e. ClangdUnitStore seem like a good place to do that)
================
Comment at: clangd/ClangdUnitStore.h:29-30
+ void runOnUnit(PathRef File, StringRef FileContents,
+ GlobalCompilationDatabase &CDB,
+ std::shared_ptr<PCHContainerOperations> PCHs, Func Action,
+ bool ReparseBeforeAction = true) {
----------------
klimek wrote:
> Not having read enough of the rest of the patch yet, I'd expect these things
> to be stored on the class level instead of being passed around all the time.
> Similarly, we're missing the VFS?
The VFS changes are not in this commit, they will follow later, that's an
initial refactoring of clangd to allow more gradual changes later.
The idea is that this class is an implementation detail of ClangdServer that
handles synchronized access to underlying ClangdUnits(which are not
thread-safe) and doesn't do anything else (i.e. doesn't care about storing the
contents of the files etc).
================
Comment at: clangd/ClangdUnitStore.h:31
+ std::shared_ptr<PCHContainerOperations> PCHs, Func Action,
+ bool ReparseBeforeAction = true) {
+ std::lock_guard<std::mutex> Lock(Mutex);
----------------
klimek wrote:
> Parameters after a function are unfortunate, as they make the code flow weird
> when you want to pass in a lambda, but still want to set the parameter after
> it :(
> Given how different the semantics are, I'd perhaps make these 2 functions:
>
> // Run on the translation unit.
> // If there was a previous call of runOnUnitWithContent, the content
> provided there will be used.
> // Otherwise, the file will be read from VFS.
> runOnStoredUnit(PathRef File, Func Action);
>
> // Run on the translation unit with the given content.
> // Content will be stored as in-flight code for the given translation unit
> only:
> // - if a different translation unit is parsed that #includes File, Content
> will not be used;
> // - subsequent calls to runOnStoredUnit will re-use Content.
> runOnUnitWithContent(PathRef File, StringRef Content, Func Action);
>
>
Moved the Func parameter to the end of the list.
The difference is semantics is different from what it seemed. Added comments
and split function into two to clarify that.
We need Content in both cases, because if there's no ClangdUnit, then we'll
have to create it and it requires initial text. And most actions need to call
'ClangdUnit::reparse' before they are run. But not code completion, which
basically does reparse itself, hence the ReparseBeforeActionParameter.
See updated code for more details.
================
Comment at: clangd/ClangdUnitStore.h:32
+ bool ReparseBeforeAction = true) {
+ std::lock_guard<std::mutex> Lock(Mutex);
+
----------------
klimek wrote:
> This is due to a restriction in the AST unit? If we don't need to reparse,
> why can't we run multiple things over an AST at once?
There are things like codeComplete that are actually mutable, but all other
things could probably be run in parallel. (Don't have a great insight into
that, though).
I really didn't want to change any of clangd behaviour in that commit, but I'd
be glad to try adding that later.
What we could have in the future:
runOnUnitUnderReadLock(... [](const ClangdUnit &Unit) { auto diags =
Unit.getLocalDiagnostics(); /* ... */} );
runOnUnitUnderWriteLock(... []( ClangdUnit &Unit) { auto completions =
Unit.codeComplete(); /* ... */} );
================
Comment at: clangd/GlobalCompilationDatabase.h:28
+/// Provides compilation arguments used for building ClangdUnit.
+class GlobalCompilationDatabase {
+public:
----------------
klimek wrote:
> What's global about this?
The fact that it's created only once(as opposed to
tooling::CompilationDatabase, which can be recreated for each file) and handles
all CompileCommand-related things(i.e. the idea is to add tracking of compile
flags changes to this interface).
I called it ClangdCompilationDatabase first, but then I thought there's too
much Clangd-prefixed names :-)
Any suggestions for a better name?
https://reviews.llvm.org/D33047
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
