sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land.
================ Comment at: clang-tools-extra/clangd/ClangdServer.h:33 #include "clang/Tooling/Core/Replacement.h" +#include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/FunctionExtras.h" ---------------- (include no longer used?) ================ Comment at: clang-tools-extra/clangd/Module.h:19 +/// - these modules are then passed to ClangdLSPServer and ClangdServer +/// FIXME: LSP bindings should be registered at ClangdLSPServer creation, and +/// we should make some server facilities like TUScheduler and index ---------------- not at creation, but rather at `initialize` time ================ Comment at: clang-tools-extra/clangd/Module.h:20 +/// FIXME: LSP bindings should be registered at ClangdLSPServer creation, and +/// we should make some server facilities like TUScheduler and index +/// available to those modules after ClangdServer is initalized. ---------------- maybe this is a separate fixme (basically ClangdLSPServer vs Server) ================ Comment at: clang-tools-extra/clangd/Module.h:22 +/// available to those modules after ClangdServer is initalized. +/// - module hooks can be called afterwards. +/// - ClangdServer will not be destroyed until all the requests are done. ---------------- after what? at this point? ================ Comment at: clang-tools-extra/clangd/Module.h:31 +public: + virtual ~Module() = default; + ---------------- Should either: - doc that destructor should cancel as much background work as possible and block until it's done - add a requestStop() and doc that destructor should block - remove the concept of background work completely (i.e. blockUntilIdle()) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96244/new/ https://reviews.llvm.org/D96244 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
