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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 16 inline comments as done.
kadircet added a comment.

Thanks for also taking a look at the implementation, it is not complete yet. I am rather waiting for a green light on struct itself, so that I can write tests.



================
Comment at: clang-tools-extra/clangd/XRefs.cpp:726
+        llvm::raw_string_ostream OS(*P.Type);
+        printSpecifiers(OS, PVD);
+        PVD->getType().print(OS, Policy);
----------------
sammccall wrote:
> what are the storage class specifiers that are relevant to function parameters?
i suppose only "register" is allowed in the context of a function parameter. but doesn't matter now as requested above, leaving specifiers out.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1209
+    else {
+      if (TemplateParameters) {
+        Output.emplace_back();
----------------
sammccall wrote:
> why do we have this no-definition case?
> Please avoid reinventing code to print C++ syntax if possible...
oops, this branch is a leftover :/


================
Comment at: clang-tools-extra/clangd/XRefs.h:89
+  llvm::Optional<std::vector<Param>> Parameters;
+  /// Set for all template declarations(function, class, variable).
+  llvm::Optional<std::vector<Param>> TemplateParameters;
----------------
sammccall wrote:
> nit: just "templates"?
> 
> (e.g. if we hover over a call to std::swap, we might be showing the instantiation rather than the declaration?)
Existing hover behavior is to show declaration, except auto, in that case we might show instantiations.

But you are right it is not necessarily only declarations.


================
Comment at: clang-tools-extra/clangd/XRefs.h:93
+  /// Lower to LSP struct.
+  Hover render() const;
+};
----------------
sammccall wrote:
> Even if this struct ends up containing the range, it might be more obvious to have render() produce the `MarkupContent` only, leaving the caller to pull out the range themselves.
> 
> Converting the range isn't closely related to rendering, and I think for cleanest layering/testing you want to return `FormattedString` after rL360151 (which doesn't have a `Hover` equivalent).
I don't think caller of this method would have enough context to deduce symbol's range so leaving the `Range` in the struct, but changing the output


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:940
           )cpp",
-          "class std::initializer_list<int>",
+          "template<> class initializer_list<int> {}",
       },
----------------
sammccall wrote:
> 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.
The old behavior was inconsistent in the case of auto. We print the decl in all cases, but print the type in the case of auto. For example, if you had 
`initializer_list<int> i = {1,2}` instead of `auto i = {1,2}` you would get the new response I've provided in this test.

I agree this looks weird in the case of instantiations but I believe it is more important to give a consistent look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61497