sammccall added a comment. This looks really nice!
================ Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:615 {"workspaceSymbolProvider", true}, {"referencesProvider", true}, {"executeCommandProvider", ---------------- can you add `{"memoryTreeProvider", true} // clangd extension`? I'd like to add client support in VSCode and it's useful if the command is only visible with versions of clangd that support it. ================ Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1389 +void ClangdLSPServer::onDumpMemoryTree(const NoParams &, + Callback<llvm::json::Value> Reply) { ---------------- Putting the serialization code inline is a tempting option here because it's (mostly) just a map. It's not something we do often though, so it does bury the protocol details in an unexpected place. A couple of alternatives to consider: - map to a `struct UsageTree { unsigned Self; unsigned Total; Map<string, UsageTree> Children; }` defined in protocol.h, and define marshalling in the usual way - skip the struct and use MemoryTree as the type, providing a toJSON(const MemoryTree&) function in protocol.h I lean towards the last alternative because it provides a bit more regularity without adding much more redundancy, but this is up to you. ================ Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1391 + Callback<llvm::json::Value> Reply) { + llvm::BumpPtrAllocator Alloc; + MemoryTree MT(&Alloc); ---------------- nit: Alloc -> DetailAlloc ================ Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1458 MsgHandler->bind("textDocument/semanticTokens/full/delta", &ClangdLSPServer::onSemanticTokensDelta); + MsgHandler->bind("$/dumpMemoryTree", &ClangdLSPServer::onDumpMemoryTree); if (Opts.FoldingRanges) ---------------- I think "dump" is a bit casual and also redundant - LSP methods to obtain information tend to have noun names. (executeCommand is the exception that proves the rule - it's used for side effects). I went through the same question with the AST-dumping and landed on textDocument/ast as my preferred option, for what it's worth. ================ Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:145 Callback<SemanticTokensOrDelta>); + /// This is a clangd extension. Provides a json tree representing memory usage + /// hierarchy. Keys starting with an underscore(_) represent leaves, e.g. ---------------- (if you decide to move toJSON to protocol.h, then the second sentence onward probably belongs there) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89277/new/ https://reviews.llvm.org/D89277 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits