Major refactoring to split LSP implementation, Clang API calls and threading(mostly synchronization)
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).
I like to put the public interface first, as that's what people usually look at first.
AFAIK usually we don't duplicate access modifiers.
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.
Tiny nit: please add empty lines between classes and namespace delcs :)
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.
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 :(
// 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);
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?
Here and elsewhere: in Clang, we use FIXME instead of TODO :)
What's global about this?
The idea is to have a class that wraps ClangdServer and is called by clangd's ProtocolHandlers.cpp to do all the work.
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.
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)
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).
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.
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).
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).
nit: we might want to keep these sorted
The combination of the name and the type of this variable makes no sense.
I feel that shouldBuildDiagnosticsForFile doesn't belong to this interface.
If this models a request to the worker thread, why not call it WorkerRequest or something? The current name is confusing, because it implies that also other requests might be handled in this way.
Maybe explicitly mention Worker in the name of this class?
The constructor already gets a ClangdServer, why pass it here?
Totally agree, renamed it.
Sorry, this sneaked in from some code that didn't end up in this commit.
I actually like the name ClangdScheduler. Albeit, it's a bit too generic and lives inside the clangd namespace, so I'm afraid of clashes later on, but until that happens I'd rather keep it.
Just to avoid introducing redundant fields.
I think it's fine to have an intermediate step checked in; the important part is that we add comments explaining why this is the way it is, especially if we plan to change it in the future :) -> I'd add a class level comment explaining what this class does and why it exists.
Tiny nit: generally, use "." at the end of sentences :)
Why don't we just use a tooling::CompilationDatabase and pass it around?
Addressed new comments
Added comments to this class. And ClangdServer, it didn't have a comment too.
It has getAllFiles and getAllCompileCommands which we don't need for now.
Also, currently implementations of CompilationDatabase actually require us to recreate them for each file, this logic is currently moved into DirectoryBasedGlobalCompilationDatabase for convenience.