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

[Krasimir Georgiev via Phabricator via cfe-commits]

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(&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;
-
-private:
-  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 {
 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 &Server, ServerRequest Request);
 
----------------
The constructor already gets a `ClangdServer`, why pass it here?


https://reviews.llvm.org/D33047