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

[Manuel Klimek via Phabricator via cfe-commits]

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 &Out;
----------------
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 &CDB,
+                 std::shared_ptr<PCHContainerOperations> 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<PCHContainerOperations> PCHs, Func Action,
+                 bool ReparseBeforeAction = true) {
+    std::lock_guard<std::mutex> 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<std::mutex> 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