sammccall added a comment.
Main comment is that I think the code is doing too much work to exactly
reproduce the current output, and include as much information as possible.
Minimizing the diff is good all else equal, but one of the goals here is to
have richer hovercards that are more consistent across the different codepaths
that generate them.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:456
+// FIXME: Expose in AST/Decl ?
+void printSpecifiers(llvm::raw_ostream &Out, const FunctionDecl *D) {
----------------
This has been pulled from DeclPrinter, so by definition duplicates info in the
printed decl. We should only do this for info that's important, and I'm not
sure most of this meets the bar.
- that a function is virtual is certainly important (but isVirtual(), not
isVirtualAsWritten())
- that the declaration we found for a function is extern is certainly
unimportant (it says something about the *decl*, not the function)
- that a function is static means *something* important, but being a
non-instance member is pretty different from being an internal helper function.
Saying Kind=StaticMethod seems more useful for presentation than putting
"static" next to the type again.
Aside from this, the current implementation puts these specifiers in the type,
which they're not - functions don't return virtual ints or static strings.
I think we can probably drop these for now, and leave them in the printed decl
only. We may want to revisit this as part of having Kind describe results more
precisely.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:726
+ llvm::raw_string_ostream OS(*P.Type);
+ printSpecifiers(OS, PVD);
+ PVD->getType().print(OS, Policy);
----------------
what are the storage class specifiers that are relevant to function parameters?
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:737
+ }
+ if (FD->isVariadic()) {
+ HI.Parameters->emplace_back();
----------------
Not totally sure this is the best way to model variadics. We could consider
leaving it out for now.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1201
+
+ if (Kind == SymbolKind::String) {
+ // We use SymbolKind::String for macro kinds
----------------
This really seems like the wrong idea to me.
"Kind" is a category description suitable to be shown to the user, it's
supposed to be useful.
In odd cases we may choose to do things like not show the type if the kind is
function.
But HoverInfo is basically a box of facts to render, and using totally
different rendering strategies for different kinds (and assuming strings mean
macros) cuts against this.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1209
+ else {
+ if (TemplateParameters) {
+ Output.emplace_back();
----------------
why do we have this no-definition case?
Please avoid reinventing code to print C++ syntax if possible...
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:940
)cpp",
- "class std::initializer_list<int>",
+ "template<> class initializer_list<int> {}",
},
----------------
hmm, this is a bit weird - this uses specialization syntax but there's no
actual specialization here right?
I think the old output without `template<>` is probably better if possible.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61497/new/
https://reviews.llvm.org/D61497
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
