[PATCH] D33047: [ClangD] Refactor clangd into separate components

[Ilya Biryukov via Phabricator via cfe-commits]

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