[PATCH] D96244: [clangd] Introduce Modules

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
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
----------------
sammccall wrote:
> 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)?
I was leaving this out from initial patch because current module interface didn't have any dependence on server facilities (hence the fixme below). Happy to declare the semantics now though, all your points are right.


================
Comment at: clang-tools-extra/clangd/Module.h:36
+  /// Identifier for the plugin, should be unique.
+  virtual llvm::StringLiteral id() = 0;
+
----------------
sammccall wrote:
> what is this for? is it needed?
no more. i was planning to make use of it to dispatch command executions to relevant module, but as we discussed offline when modules register those themselves it won't be 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() {}
----------------
sammccall wrote:
> Some indication that this is for testing.
this is needed to block clangdserver shutdown until modules finish any background tasks. would it make sense to rename it to `stop` instead?


================
Comment at: clang-tools-extra/clangd/Module.h:43
+
+/// Wrapper around a set of modules to provide type based grouping.
+// Ideas for implementing this:
----------------
sammccall wrote:
> 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)
SG. I've copied the iterators from your patch :)


================
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.
----------------
sammccall wrote:
> this interface isn't compatible with modules eventually having public interfaces, because it loses the type information. but we can change this later
agreed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96244