[PATCH] D96751: [clangd] Populate detail field in document symbols

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for doing this!

I think this behavior is a great starting point.

I suspect printing the decl is more verbose than desired in a lot of cases, at least avoiding repeating the identifier would be nice. e.g.
x | int x --> x | int
foo | void foo() --> foo | void()
bar | struct bar {} --> bar | struct
This can be tweaked later, the simple implementation seems like a big improvement already.
If you feel like it though, I think this is mostly just printing the type instead of the decl for ValueDecls.

When you upload snapshots, please prepare the diffs with full context `-U9999` to make review easier. (the Arcanist `arc` tool will do this automatically)



================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:201
+  P.SuppressUnwrittenScope = true;
+  llvm::raw_string_ostream OS(SI.detail);
+  if (const auto *TP = ND.getDescribedTemplateParams()) {
----------------
nit: limit the scope of raw_string_ostream by putting it in an anonymous block.
It does not flush until destroyed (or explicitly flushed)


================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:201
+  P.SuppressUnwrittenScope = true;
+  llvm::raw_string_ostream OS(SI.detail);
+  if (const auto *TP = ND.getDescribedTemplateParams()) {
----------------
sammccall wrote:
> nit: limit the scope of raw_string_ostream by putting it in an anonymous block.
> It does not flush until destroyed (or explicitly flushed)
Can we pull out a helper function for this? getSymbolDetail or so?

For now it's just a few lines but I imagine it could grow as we refine the behavior.


================
Comment at: clang-tools-extra/clangd/Hover.h:20
 
+PrintingPolicy getPrintingPolicy(PrintingPolicy Base);
 /// Contains detailed information about a Symbol. Especially useful when
----------------
this isn't a reasonable thing to expose & depend on from FindSymbols.cpp.

It's fine to add a copy into FindSymbols.cpp, especially since you want a slightly different policy anyway


================
Comment at: clang-tools-extra/clangd/test/symbols.test:1
 # RUN: clangd --index-file=%S/Inputs/symbols.test.yaml -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"documentSymbol":{"hierarchicalDocumentSymbolSupport":true}},"workspace":{"symbol":{"symbolKind":{"valueSet": [1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26]}}}},"trace":"off"}}
----------------
Can you add unit-tests for this feature in FindSymbolsTests.cpp?
Don't need to assert the detail for every symbol we test there, but adding a WithDetail matcher and adding it to a few nodes in the testcases would be useful. In particular, at least one that shows the getDescribedTemplateParams bit.

(It's nice to have a basic smoke-test here but unit-tests are where we try to cover more of the cases)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96751