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