[PATCH] D89277: [clangd] Add $/dumpMemoryTree LSP extension

[Sam McCall via Phabricator via cfe-commits]

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