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 cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits