[PATCH] D31746: Remove ASTUnits for closed documents and cache CompilationDatabase per directory in clangd.

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added a comment.

Looks great! I'm wondering, can you think of ways to test the `didClose` method similarly to how it's done for other handlers?



================
Comment at: clangd/ASTManager.cpp:203
+  // TODO(ibiryukov): at this point DocDatasLock can be unlocked in asynchronous
+  // mode
 
----------------
Why not make the locking explicit here and don't require `handleRequest` and `parseFileAndPublishDiagnostics` to be called under a lock?


================
Comment at: clangd/ASTManager.h:60
+  ASTManagerRequest() = default;
+  ASTManagerRequest(ASTManagerRequestType Type, std::string FileUri,
+                    DocVersion FileVersion);
----------------
I'd call the second parameter just `Uri`.


================
Comment at: clangd/ASTManager.h:64
+  ASTManagerRequestType type;
+  std::string fileUri;
+  DocVersion fileVersion;
----------------
I'd call this `Uri`.


================
Comment at: clangd/ASTManager.h:65
+  std::string fileUri;
+  DocVersion fileVersion;
+};
----------------
I'd capitalize the member variables. In general, this is the llvm/clang style and the lowercase member variables in `Protocol.h` are for consistency with the Language Server Protocol.


================
Comment at: clangd/ASTManager.h:112
+  /// thread.
+  /// If RunSynchronously is true, runs the request handler immideately
+  void queueOrRun(ASTManagerRequestType RequestType, StringRef Uri);
----------------
nit: `s/immideately/immediately` and add a full stop.


================
Comment at: clangd/ASTManager.h:117
+  /// Should be called under DocDataLock when RunSynchronously is false
+  void handleRequest(ASTManagerRequestType RequestType, StringRef Uri);
+  /// Should be called under DocDataLock when RunSynchronously is false
----------------
It's sad that handle request should be called under the lock :(


================
Comment at: clangd/ASTManager.h:119
+  /// Should be called under DocDataLock when RunSynchronously is false
+  void parseFileAndPublishDiagnostics(StringRef Uri);
 
----------------
Maybe it's worth noting whether the main or the worker thread calls these during asynchronous execution.


================
Comment at: clangd/ASTManager.h:131
   std::shared_ptr<clang::PCHContainerOperations> PCHs;
+  /// A lock for access to the DocDatas, CompilationDatabases and PCHs
+  std::mutex DocDatasLock;
----------------
nit: missing trailing full stop.


================
Comment at: clangd/ASTManager.h:132
+  /// A lock for access to the DocDatas, CompilationDatabases and PCHs
+  std::mutex DocDatasLock;
 
----------------
I'd rename this lock to `ClangObjectLock` or something else; its name currently gives the wrong feeling that it's used only for `DocDatas`.


https://reviews.llvm.org/D31746