[PATCH] D33047: [ClangD] Refactor clangd into separate components
bkramer accepted this revision. bkramer added a comment. This revision is now accepted and ready to land. I believe this is good enough now. https://reviews.llvm.org/D33047 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33047: [ClangD] Refactor clangd into separate components
ilya-biryukov added inline comments.
Comment at: clangd/ClangdLSPServer.h:23
+
+class ClangdLSPServer {
+ class LSPDiagnosticsConsumer;
klimek wrote:
> ilya-biryukov wrote:
> > 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.
> 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.
Added comments to this class. And ClangdServer, it didn't have a comment too.
Comment at: clangd/ClangdUnit.h:43
+
+ /// Reparse with new contents
+ void reparse(StringRef Contents);
klimek wrote:
> Tiny nit: generally, use "." at the end of sentences :)
Fixed
Comment at: clangd/GlobalCompilationDatabase.h:28
+/// Provides compilation arguments used for building ClangdUnit.
+class GlobalCompilationDatabase {
+public:
klimek wrote:
> ilya-biryukov wrote:
> > 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?
> Why don't we just use a tooling::CompilationDatabase and pass it around?
It has getAllFiles and getAllCompileCommands which we don't need for now.
It doesn't have capabilities to track changes to flags, which we'll want to add
at some point. (Added a comment that we'll be adding those).
Also, currently implementations of CompilationDatabase actually require us to
recreate them for each file, this logic is currently moved into
DirectoryBasedGlobalCompilationDatabase for convenience.
https://reviews.llvm.org/D33047
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33047: [ClangD] Refactor clangd into separate components
ilya-biryukov updated this revision to Diff 98745.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
Addressed new comments
https://reviews.llvm.org/D33047
Files:
clangd/ASTManager.cpp
clangd/ASTManager.h
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdMain.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/ClangdUnitStore.cpp
clangd/ClangdUnitStore.h
clangd/DocumentStore.h
clangd/DraftStore.cpp
clangd/DraftStore.h
clangd/GlobalCompilationDatabase.cpp
clangd/GlobalCompilationDatabase.h
clangd/Path.h
clangd/ProtocolHandlers.cpp
clangd/ProtocolHandlers.h
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -22,8 +22,8 @@
namespace clang {
namespace clangd {
-class ASTManager;
-class DocumentStore;
+class ClangdLSPServer;
+class ClangdLSPServer;
struct InitializeHandler : Handler {
InitializeHandler(JSONOutput ) : Handler(Output) {}
@@ -56,83 +56,83 @@
};
struct TextDocumentDidOpenHandler : Handler {
- TextDocumentDidOpenHandler(JSONOutput , DocumentStore )
- : Handler(Output), Store(Store) {}
+ TextDocumentDidOpenHandler(JSONOutput , ClangdLSPServer )
+ : Handler(Output), AST(AST) {}
void handleNotification(llvm::yaml::MappingNode *Params) override;
private:
- DocumentStore
+ ClangdLSPServer
};
struct TextDocumentDidChangeHandler : Handler {
- TextDocumentDidChangeHandler(JSONOutput , DocumentStore )
- : Handler(Output), Store(Store) {}
+ TextDocumentDidChangeHandler(JSONOutput , ClangdLSPServer )
+ : Handler(Output), AST(AST) {}
void handleNotification(llvm::yaml::MappingNode *Params) override;
private:
- DocumentStore
+ ClangdLSPServer
};
struct TextDocumentDidCloseHandler : Handler {
- TextDocumentDidCloseHandler(JSONOutput , DocumentStore )
- : Handler(Output), Store(Store) {}
+ TextDocumentDidCloseHandler(JSONOutput , ClangdLSPServer )
+ : Handler(Output), AST(AST) {}
void handleNotification(llvm::yaml::MappingNode *Params) override;
private:
- DocumentStore
+ ClangdLSPServer
};
struct TextDocumentOnTypeFormattingHandler : Handler {
- TextDocumentOnTypeFormattingHandler(JSONOutput , DocumentStore )
- : Handler(Output), Store(Store) {}
+ TextDocumentOnTypeFormattingHandler(JSONOutput , ClangdLSPServer )
+ : Handler(Output), AST(AST) {}
void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override;
private:
- DocumentStore
+ ClangdLSPServer
};
struct TextDocumentRangeFormattingHandler : Handler {
- TextDocumentRangeFormattingHandler(JSONOutput , DocumentStore )
- : Handler(Output), Store(Store) {}
+ TextDocumentRangeFormattingHandler(JSONOutput , ClangdLSPServer )
+ : Handler(Output), AST(AST) {}
void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override;
private:
- DocumentStore
+ ClangdLSPServer
};
struct TextDocumentFormattingHandler : Handler {
- TextDocumentFormattingHandler(JSONOutput , DocumentStore )
- : Handler(Output), Store(Store) {}
+ TextDocumentFormattingHandler(JSONOutput , ClangdLSPServer )
+ : Handler(Output), AST(AST) {}
void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override;
private:
- DocumentStore
+ ClangdLSPServer
};
struct CodeActionHandler : Handler {
- CodeActionHandler(JSONOutput , ASTManager )
+ CodeActionHandler(JSONOutput , ClangdLSPServer )
: Handler(Output), AST(AST) {}
void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override;
private:
- ASTManager
+ ClangdLSPServer
};
struct CompletionHandler : Handler {
- CompletionHandler(JSONOutput , ASTManager )
+ CompletionHandler(JSONOutput , ClangdLSPServer )
: Handler(Output), AST(AST) {}
void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override;
private:
- ASTManager
+ ClangdLSPServer
};
} // namespace clangd
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -8,9 +8,10 @@
//===--===//
#include "ProtocolHandlers.h"
-#include "ASTManager.h"
-#include "DocumentStore.h"
+#include "ClangdServer.h"
+#include "DraftStore.h"
#include "clang/Format/Format.h"
+#include "ClangdLSPServer.h"
using namespace clang;
using namespace clangd;
@@ -21,7 +22,7 @@
Output.log("Failed to decode DidOpenTextDocumentParams!\n");
return;
}
- Store.addDocument(DOTDP->textDocument.uri.file, DOTDP->textDocument.text);
+ AST.openDocument(DOTDP->textDocument.uri.file, DOTDP->textDocument.text);
}
void TextDocumentDidCloseHandler::handleNotification(
@@ -32,7 +33,7 @@
[PATCH] D33047: [ClangD] Refactor clangd into separate components
klimek added inline comments.
Comment at: clangd/ClangdLSPServer.h:23
+
+class ClangdLSPServer {
+ class LSPDiagnosticsConsumer;
ilya-biryukov wrote:
> 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.
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.
Comment at: clangd/ClangdUnit.h:43
+
+ /// Reparse with new contents
+ void reparse(StringRef Contents);
Tiny nit: generally, use "." at the end of sentences :)
Comment at: clangd/GlobalCompilationDatabase.h:28
+/// Provides compilation arguments used for building ClangdUnit.
+class GlobalCompilationDatabase {
+public:
ilya-biryukov wrote:
> 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?
Why don't we just use a tooling::CompilationDatabase and pass it around?
https://reviews.llvm.org/D33047
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33047: [ClangD] Refactor clangd into separate components
ilya-biryukov added inline comments.
Comment at: clangd/ClangdMain.cpp:41
// dispatching.
- DocumentStore Store;
- ASTManager AST(Out, Store, RunSynchronously);
- Store.addListener();
+ ClangdLSPServer AST(Out, RunSynchronously);
JSONRPCDispatcher Dispatcher(llvm::make_unique(Out));
krasimir wrote:
> The combination of the name and the type of this variable makes no sense.
Totally agree, renamed it.
Comment at: clangd/ClangdServer.h:41
- void cacheFixIts(DiagnosticToReplacementMap FixIts);
- std::vector
- getFixIts(const clangd::Diagnostic ) const;
-
-private:
- std::unique_ptr AST;
- DiagnosticToReplacementMap FixIts;
+ virtual bool shouldBuildDiagnosticsForFile(PathRef File) = 0;
+ virtual void onDiagnosticsReady(PathRef File,
krasimir wrote:
> I feel that `shouldBuildDiagnosticsForFile` doesn't belong to this interface.
Sorry, this sneaked in from some code that didn't end up in this commit.
It's also not used anywhere now. I've removed it from this change.
Comment at: clangd/ClangdServer.h:62
+/// Handles scheduling on a different thread for ClangdServer
+class ClangdScheduler {
+public:
krasimir wrote:
> Maybe explicitly mention `Worker` in the name of this class?
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.
Do you find ClangdScheduler confusing? Does the comment help?
But if you feel stongly that we shall change the name, let's do it.
Comment at: clangd/ClangdServer.h:68
+ /// Enqueue ServerRequest to be run on a worker thread
+ void Enqueue(ClangdServer , ServerRequest Request);
krasimir wrote:
> The constructor already gets a `ClangdServer`, why pass it here?
Just to avoid introducing redundant fields.
(I.e. ClangdServer stores ClangdScheduler, which stores a reference to
ClangdServer).
https://reviews.llvm.org/D33047
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33047: [ClangD] Refactor clangd into separate components
ilya-biryukov updated this revision to Diff 98634.
ilya-biryukov marked 3 inline comments as done.
ilya-biryukov added a comment.
Addressed style comments.
Moved ClangdScheduler to be the last field of the ClangdServer, so
that it's constructed last and destructed first.
https://reviews.llvm.org/D33047
Files:
clangd/ASTManager.cpp
clangd/ASTManager.h
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdMain.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/ClangdUnitStore.cpp
clangd/ClangdUnitStore.h
clangd/DocumentStore.h
clangd/DraftStore.cpp
clangd/DraftStore.h
clangd/GlobalCompilationDatabase.cpp
clangd/GlobalCompilationDatabase.h
clangd/Path.h
clangd/ProtocolHandlers.cpp
clangd/ProtocolHandlers.h
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -22,8 +22,8 @@
namespace clang {
namespace clangd {
-class ASTManager;
-class DocumentStore;
+class ClangdLSPServer;
+class ClangdLSPServer;
struct InitializeHandler : Handler {
InitializeHandler(JSONOutput ) : Handler(Output) {}
@@ -56,83 +56,83 @@
};
struct TextDocumentDidOpenHandler : Handler {
- TextDocumentDidOpenHandler(JSONOutput , DocumentStore )
- : Handler(Output), Store(Store) {}
+ TextDocumentDidOpenHandler(JSONOutput , ClangdLSPServer )
+ : Handler(Output), AST(AST) {}
void handleNotification(llvm::yaml::MappingNode *Params) override;
private:
- DocumentStore
+ ClangdLSPServer
};
struct TextDocumentDidChangeHandler : Handler {
- TextDocumentDidChangeHandler(JSONOutput , DocumentStore )
- : Handler(Output), Store(Store) {}
+ TextDocumentDidChangeHandler(JSONOutput , ClangdLSPServer )
+ : Handler(Output), AST(AST) {}
void handleNotification(llvm::yaml::MappingNode *Params) override;
private:
- DocumentStore
+ ClangdLSPServer
};
struct TextDocumentDidCloseHandler : Handler {
- TextDocumentDidCloseHandler(JSONOutput , DocumentStore )
- : Handler(Output), Store(Store) {}
+ TextDocumentDidCloseHandler(JSONOutput , ClangdLSPServer )
+ : Handler(Output), AST(AST) {}
void handleNotification(llvm::yaml::MappingNode *Params) override;
private:
- DocumentStore
+ ClangdLSPServer
};
struct TextDocumentOnTypeFormattingHandler : Handler {
- TextDocumentOnTypeFormattingHandler(JSONOutput , DocumentStore )
- : Handler(Output), Store(Store) {}
+ TextDocumentOnTypeFormattingHandler(JSONOutput , ClangdLSPServer )
+ : Handler(Output), AST(AST) {}
void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override;
private:
- DocumentStore
+ ClangdLSPServer
};
struct TextDocumentRangeFormattingHandler : Handler {
- TextDocumentRangeFormattingHandler(JSONOutput , DocumentStore )
- : Handler(Output), Store(Store) {}
+ TextDocumentRangeFormattingHandler(JSONOutput , ClangdLSPServer )
+ : Handler(Output), AST(AST) {}
void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override;
private:
- DocumentStore
+ ClangdLSPServer
};
struct TextDocumentFormattingHandler : Handler {
- TextDocumentFormattingHandler(JSONOutput , DocumentStore )
- : Handler(Output), Store(Store) {}
+ TextDocumentFormattingHandler(JSONOutput , ClangdLSPServer )
+ : Handler(Output), AST(AST) {}
void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override;
private:
- DocumentStore
+ ClangdLSPServer
};
struct CodeActionHandler : Handler {
- CodeActionHandler(JSONOutput , ASTManager )
+ CodeActionHandler(JSONOutput , ClangdLSPServer )
: Handler(Output), AST(AST) {}
void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override;
private:
- ASTManager
+ ClangdLSPServer
};
struct CompletionHandler : Handler {
- CompletionHandler(JSONOutput , ASTManager )
+ CompletionHandler(JSONOutput , ClangdLSPServer )
: Handler(Output), AST(AST) {}
void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override;
private:
- ASTManager
+ ClangdLSPServer
};
} // namespace clangd
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -8,9 +8,10 @@
//===--===//
#include "ProtocolHandlers.h"
-#include "ASTManager.h"
-#include "DocumentStore.h"
+#include "ClangdServer.h"
+#include "DraftStore.h"
#include "clang/Format/Format.h"
+#include "ClangdLSPServer.h"
using namespace clang;
using namespace clangd;
@@ -21,7 +22,7 @@
Output.log("Failed to decode DidOpenTextDocumentParams!\n");
return;
}
- Store.addDocument(DOTDP->textDocument.uri.file, DOTDP->textDocument.text);
+
[PATCH] D33047: [ClangD] Refactor clangd into separate components
krasimir added inline comments.
Comment at: clangd/CMakeLists.txt:12
ProtocolHandlers.cpp
)
nit: we might want to keep these sorted
Comment at: clangd/ClangdMain.cpp:41
// dispatching.
- DocumentStore Store;
- ASTManager AST(Out, Store, RunSynchronously);
- Store.addListener();
+ ClangdLSPServer AST(Out, RunSynchronously);
JSONRPCDispatcher Dispatcher(llvm::make_unique(Out));
The combination of the name and the type of this variable makes no sense.
Comment at: clangd/ClangdServer.h:41
- void cacheFixIts(DiagnosticToReplacementMap FixIts);
- std::vector
- getFixIts(const clangd::Diagnostic ) const;
-
-private:
- std::unique_ptr AST;
- DiagnosticToReplacementMap FixIts;
+ virtual bool shouldBuildDiagnosticsForFile(PathRef File) = 0;
+ virtual void onDiagnosticsReady(PathRef File,
I feel that `shouldBuildDiagnosticsForFile` doesn't belong to this interface.
Comment at: clangd/ClangdServer.h:49
/// A request to the worker thread
-class ASTManagerRequest {
+class ServerRequest {
public:
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.
Comment at: clangd/ClangdServer.h:62
+/// Handles scheduling on a different thread for ClangdServer
+class ClangdScheduler {
+public:
Maybe explicitly mention `Worker` in the name of this class?
Comment at: clangd/ClangdServer.h:68
+ /// Enqueue ServerRequest to be run on a worker thread
+ void Enqueue(ClangdServer , ServerRequest Request);
The constructor already gets a `ClangdServer`, why pass it here?
https://reviews.llvm.org/D33047
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33047: [ClangD] Refactor clangd into separate components
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 ,
+ std::shared_ptr 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 PCHs, Func Action,
+ bool ReparseBeforeAction = true) {
+std::lock_guard 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 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 ) { auto diags =
Unit.getLocalDiagnostics(); /* ... */} );
runOnUnitUnderWriteLock(... []( ClangdUnit ) { 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.
[PATCH] D33047: [ClangD] Refactor clangd into separate components
ilya-biryukov updated this revision to Diff 98606.
ilya-biryukov marked 3 inline comments as done.
ilya-biryukov added a comment.
Addressed review comments
https://reviews.llvm.org/D33047
Files:
clangd/ASTManager.cpp
clangd/ASTManager.h
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdMain.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/ClangdUnitStore.cpp
clangd/ClangdUnitStore.h
clangd/DocumentStore.h
clangd/DraftStore.cpp
clangd/DraftStore.h
clangd/GlobalCompilationDatabase.cpp
clangd/GlobalCompilationDatabase.h
clangd/Path.h
clangd/ProtocolHandlers.cpp
clangd/ProtocolHandlers.h
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -22,8 +22,8 @@
namespace clang {
namespace clangd {
-class ASTManager;
-class DocumentStore;
+class ClangdLSPServer;
+class ClangdLSPServer;
struct InitializeHandler : Handler {
InitializeHandler(JSONOutput ) : Handler(Output) {}
@@ -56,83 +56,83 @@
};
struct TextDocumentDidOpenHandler : Handler {
- TextDocumentDidOpenHandler(JSONOutput , DocumentStore )
- : Handler(Output), Store(Store) {}
+ TextDocumentDidOpenHandler(JSONOutput , ClangdLSPServer )
+ : Handler(Output), AST(AST) {}
void handleNotification(llvm::yaml::MappingNode *Params) override;
private:
- DocumentStore
+ ClangdLSPServer
};
struct TextDocumentDidChangeHandler : Handler {
- TextDocumentDidChangeHandler(JSONOutput , DocumentStore )
- : Handler(Output), Store(Store) {}
+ TextDocumentDidChangeHandler(JSONOutput , ClangdLSPServer )
+ : Handler(Output), AST(AST) {}
void handleNotification(llvm::yaml::MappingNode *Params) override;
private:
- DocumentStore
+ ClangdLSPServer
};
struct TextDocumentDidCloseHandler : Handler {
- TextDocumentDidCloseHandler(JSONOutput , DocumentStore )
- : Handler(Output), Store(Store) {}
+ TextDocumentDidCloseHandler(JSONOutput , ClangdLSPServer )
+ : Handler(Output), AST(AST) {}
void handleNotification(llvm::yaml::MappingNode *Params) override;
private:
- DocumentStore
+ ClangdLSPServer
};
struct TextDocumentOnTypeFormattingHandler : Handler {
- TextDocumentOnTypeFormattingHandler(JSONOutput , DocumentStore )
- : Handler(Output), Store(Store) {}
+ TextDocumentOnTypeFormattingHandler(JSONOutput , ClangdLSPServer )
+ : Handler(Output), AST(AST) {}
void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override;
private:
- DocumentStore
+ ClangdLSPServer
};
struct TextDocumentRangeFormattingHandler : Handler {
- TextDocumentRangeFormattingHandler(JSONOutput , DocumentStore )
- : Handler(Output), Store(Store) {}
+ TextDocumentRangeFormattingHandler(JSONOutput , ClangdLSPServer )
+ : Handler(Output), AST(AST) {}
void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override;
private:
- DocumentStore
+ ClangdLSPServer
};
struct TextDocumentFormattingHandler : Handler {
- TextDocumentFormattingHandler(JSONOutput , DocumentStore )
- : Handler(Output), Store(Store) {}
+ TextDocumentFormattingHandler(JSONOutput , ClangdLSPServer )
+ : Handler(Output), AST(AST) {}
void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override;
private:
- DocumentStore
+ ClangdLSPServer
};
struct CodeActionHandler : Handler {
- CodeActionHandler(JSONOutput , ASTManager )
+ CodeActionHandler(JSONOutput , ClangdLSPServer )
: Handler(Output), AST(AST) {}
void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override;
private:
- ASTManager
+ ClangdLSPServer
};
struct CompletionHandler : Handler {
- CompletionHandler(JSONOutput , ASTManager )
+ CompletionHandler(JSONOutput , ClangdLSPServer )
: Handler(Output), AST(AST) {}
void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override;
private:
- ASTManager
+ ClangdLSPServer
};
} // namespace clangd
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -8,9 +8,10 @@
//===--===//
#include "ProtocolHandlers.h"
-#include "ASTManager.h"
-#include "DocumentStore.h"
+#include "ClangdServer.h"
+#include "DraftStore.h"
#include "clang/Format/Format.h"
+#include "ClangdLSPServer.h"
using namespace clang;
using namespace clangd;
@@ -21,7 +22,7 @@
Output.log("Failed to decode DidOpenTextDocumentParams!\n");
return;
}
- Store.addDocument(DOTDP->textDocument.uri.file, DOTDP->textDocument.text);
+ AST.openDocument(DOTDP->textDocument.uri.file, DOTDP->textDocument.text);
}
void TextDocumentDidCloseHandler::handleNotification(
@@ -32,7 +33,7 @@
[PATCH] D33047: [ClangD] Refactor clangd into separate components
klimek added inline comments.
Comment at: clangd/ClangdLSPServer.h:23
+
+class ClangdLSPServer {
+ class LSPDiagnosticsConsumer;
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).
Comment at: clangd/ClangdLSPServer.h:24-25
+class ClangdLSPServer {
+ class LSPDiagnosticsConsumer;
+
+public:
I like to put the public interface first, as that's what people usually look at
first.
Comment at: clangd/ClangdLSPServer.h:48
+
+private:
+ JSONOutput
AFAIK usually we don't duplicate access modifiers.
Comment at: clangd/ClangdUnit.h:44
+ /// Reparse with new contents
+ void reparse(StringRef Contents);
+
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.
Comment at: clangd/ClangdUnitStore.h:21
+namespace clang {
+namespace clangd {
+/// Thread-safe collection of ASTs built for specific files. Provides
Tiny nit: please add empty lines between classes and namespace delcs :)
Comment at: clangd/ClangdUnitStore.h:29-30
+ void runOnUnit(PathRef File, StringRef FileContents,
+ GlobalCompilationDatabase ,
+ std::shared_ptr PCHs, Func Action,
+ bool ReparseBeforeAction = true) {
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?
Comment at: clangd/ClangdUnitStore.h:31
+ std::shared_ptr PCHs, Func Action,
+ bool ReparseBeforeAction = true) {
+std::lock_guard Lock(Mutex);
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);
Comment at: clangd/ClangdUnitStore.h:32
+ bool ReparseBeforeAction = true) {
+std::lock_guard Lock(Mutex);
+
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?
Comment at: clangd/GlobalCompilationDatabase.cpp:55
+
+// TODO(ibiryukov): Invalidate cached compilation databases on changes
+auto result = CDB.get();
Here and elsewhere: in Clang, we use FIXME instead of TODO :)
Comment at: clangd/GlobalCompilationDatabase.h:28
+/// Provides compilation arguments used for building ClangdUnit.
+class GlobalCompilationDatabase {
+public:
What's global about this?
https://reviews.llvm.org/D33047
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
