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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
