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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 10 inline comments as done.
kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:567
+    const HoverInfo Expected;
+  } // namespace
+  Cases[] = {
----------------
sammccall wrote:
> namespace?!
> 
> might be a clang-format bug?
I must have messed the brackets at some point :D


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:574
+        )cpp",
+       {/*NamespaceScope=*/std::string(""),
+        /*LocalScope=*/std::string(""),
----------------
sammccall wrote:
> using the constructor/aggregate init here has a couple of downsides:
>  - vebosity: you need to list every field, even those that are not set for this case (TemplateParameters), or are misleading (SymRange is none)
>  - brittleness: it makes it hard to change the struct at all
>  - readability: the /*foo=*/ comments aren't bad, but not ideal either
> 
> Using field-by-field initialization would be better I think, though it's a bit awkward here.
> 
> but e.g. you could make the member std::function<HoverInfo> ExpectedBuilder, and write
> 
> ```
> 
> [&](HoverInfo &Expected) {
>   Expected.Name = "foo";
>   Expected.Kind = SymbolKind::Function;
>   ...
> }
> ```
yeah that looks a lot better, thanks!


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:592
+        )cpp",
+       {/*NamespaceScope=*/std::string("ns1::ns2"),
+        /*LocalScope=*/std::string(""),
----------------
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.


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:599
+        /*Definition=*/"void foo()",
+        /*Type=*/llvm::None,
+        /*ReturnType=*/std::string("void"),
----------------
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)


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:782
+        /*Definition=*/
+        "auto lamb = [&bar](int T, bool B) -> bool {\n    return T && B && "
+        "bar;\n}",
----------------
sammccall wrote:
> is it easy to omit the body?
yes, sent out D62487


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:951
           )cpp",
-          "Declared in namespace ns1\n\nstruct MyClass {}",
+          "Declared in ns1\n\nstruct MyClass {}",
       },
----------------
sammccall wrote:
> note that if you want to it's easy to keep "Declared in namespace" - the LocalScope is empty.
> (Though we can't provide this info for non-namespace containers)
I believe it doesn't make sense to add only one, so leaving it until we hear from users that they need the kind information


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61497