[PATCH] D61497: [clangd] Introduce a structured hover response

[Sam McCall via Phabricator via cfe-commits]

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