[PATCH] D96244: [clangd] Introduce Modules

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I'm a little bit nervous about adding this with *no* usage, but it keeps the patch size down :-)



================
Comment at: clang-tools-extra/clangd/Module.cpp:1
+//===--- Module.cpp - Main clangd server code --------------------*- C++-*-===//
+//
----------------
please add if/when it's actually needed


================
Comment at: clang-tools-extra/clangd/Module.h:13
+/// of only a public interface to access the functionality from C++ embedders,
+///    ClangdServer::getModule<FooModule>()->foo(...)
+///
----------------
This isn't implemented - i'm not sure if i should be reviewing the comment or the impl :-)

we'll need it at some point (if we're going to move features which have public APIs in clangdserver into modules) but not in this patch


================
Comment at: clang-tools-extra/clangd/Module.h:15
+///
+/// FIXME: Extend this with LSP bindings to support updating capabilities and
+/// implementing LSP endpoints.
----------------
nit: not just updating but also reading client caps


================
Comment at: clang-tools-extra/clangd/Module.h:24
+///    available to those modules after ClangdServer is initalized.
+///  - module hooks can be called afterwards.
+///  - modules can be destroyed before/after ClangdServer and ClangdLSPServer
----------------
need some guarantee that module *usage* ends before clangdserver is destroyed, for some sensible definition of usage


================
Comment at: clang-tools-extra/clangd/Module.h:25
+///  - module hooks can be called afterwards.
+///  - modules can be destroyed before/after ClangdServer and ClangdLSPServer
+///    FIXME: Once we make server facilities available to modules, we'll need to
----------------
this doesn't make sense to me - neither of these own the modules (right?), so who would be destroying them while the server is still alive (and how would we ensure this is safe)?


================
Comment at: clang-tools-extra/clangd/Module.h:36
+  /// Identifier for the plugin, should be unique.
+  virtual llvm::StringLiteral id() = 0;
+
----------------
what is this for? is it needed?


================
Comment at: clang-tools-extra/clangd/Module.h:39
+  /// Some modules might own background tasks. They should override this method
+  /// to indicate status of these tasks.
+  virtual void blockUntilIdle() {}
----------------
Some indication that this is for testing.


================
Comment at: clang-tools-extra/clangd/Module.h:40
+  /// to indicate status of these tasks.
+  virtual void blockUntilIdle() {}
+};
----------------
this isn't called - call it or leave it out?


================
Comment at: clang-tools-extra/clangd/Module.h:43
+
+/// Wrapper around a set of modules to provide type based grouping.
+// Ideas for implementing this:
----------------
As discussed offline, I don't think sub-interfaces are a great way to express the various ways to express clangd, at least initially.
You end up either having to deal with multiple inheritance or multiple module instances.
The former is a hassle we only need consider if the interface gets wide, the latter loses some of the simplicity of the model (if a feature contains 3 hooks that share state, where does that state go?)

For now, I think it's enough to be able to iterate over Module&s (Given the ModuleSet name, I think defining begin()/end() etc make sense)


================
Comment at: clang-tools-extra/clangd/Module.h:53
+public:
+  explicit ModuleSet(std::vector<std::unique_ptr<Module>> Modules);
+  // Returns all the modules of type T.
----------------
this interface isn't compatible with modules eventually having public interfaces, because it loses the type information. but we can change this later


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96244/new/

https://reviews.llvm.org/D96244