[PATCH] D96244: [clangd] Introduce Modules

[Sam McCall via Phabricator via cfe-commits]

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