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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Still LG

I forgot to mention - it would be nice to clang-format the definition, what do you think?



================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:592
+        )cpp",
+       {/*NamespaceScope=*/std::string("ns1::ns2"),
+        /*LocalScope=*/std::string(""),
----------------
kadircet wrote:
> sammccall wrote:
> > I think this should be "ns1::ns2::", as we use scope internally.
> > This means we can simply concatenate parts to form a qname.
> > 
> > For the current rendering, it's easy to strip ::
> should it also be "::" for global namespace ? which would also result in prefixing any symbol in global namespace with "::" when printing.
We tend to use empty string for global namespace, and explicitly add it where we need it.
Using :: for global causes as many problems as it solves (e.g. in C).


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:599
+        /*Definition=*/"void foo()",
+        /*Type=*/llvm::None,
+        /*ReturnType=*/std::string("void"),
----------------
kadircet wrote:
> sammccall wrote:
> > It would be nice to add `void()` or `void (&)()` or so if it's easy.
> > This is what `:YcmCompleter GetType` would show
> just put the type without any parameter names, but I am not sure whether users would want that. I believe people find current hover behavior a lot more useful then just showing type(which is done by libclang)
I don't think we should include it in the actual hover, but we can handle that in rendering (if it's a Function, don't print the type).
It does seem odd not to include it in the structured API, but up to you.

I thought specifically the `:GetType` might still be useful, but maybe not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61497