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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
