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

Krasimir Georgiev via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu May 11 06:44:08 PDT 2017

krasimir added inline comments.

Comment at: clangd/CMakeLists.txt:12
nit: we might want to keep these sorted

Comment at: clangd/ClangdMain.cpp:41
   // dispatching.
-  DocumentStore Store;
-  ASTManager AST(Out, Store, RunSynchronously);
-  Store.addListener(&AST);
+  ClangdLSPServer AST(Out, RunSynchronously);
   JSONRPCDispatcher Dispatcher(llvm::make_unique<Handler>(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<clang::tooling::Replacement>
-  getFixIts(const clangd::Diagnostic &D) const;
-  std::unique_ptr<ASTUnit> 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 {
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 {
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 &Server, ServerRequest Request);
The constructor already gets a `ClangdServer`, why pass it here?


More information about the cfe-commits mailing list