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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
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));
----------------
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<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,
----------------
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 &Server, 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