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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Layering and such looks good. This should compose well with D58547 <https://reviews.llvm.org/D58547>



================
Comment at: clang-tools-extra/clangd/XRefs.h:52
+struct HoverInfo {
+  LocatedSymbol Sym;
+  /// Name of the context containing the symbol.
----------------
I'm not sure about reuse of LocatedSymbol - do we want to commit to returning decl/def locations?
Name might be enough here.


================
Comment at: clang-tools-extra/clangd/XRefs.h:54
+  /// Name of the context containing the symbol.
+  std::string ContainerName;
+  index::SymbolInfo SI;
----------------
This comes from LSP, and within the scope of C++ I think we might want stronger semantics here.
I think it's likely we actually want to show namespace vs class scope differently, too - maybe these should be separate fields?


================
Comment at: clang-tools-extra/clangd/XRefs.h:55
+  std::string ContainerName;
+  index::SymbolInfo SI;
+  /// Includes all the qualifiers.
----------------
SymbolInfo is a bit of a mess. Maybe we just want SymbolInfo::Kind? (LSP SymbolKind is tempting but loses a lot of info for C++).

I do think we'll need to extend SymbolInfo::Kind or eventually use our own enum, e.g. lambdas may need their own kind (they're variables, but don't have a printable type and need to be displayed more like functions)


================
Comment at: clang-tools-extra/clangd/XRefs.h:57
+  /// Includes all the qualifiers.
+  std::string Type;
+  /// Empty for non-functions. First element is the return type.
----------------
I think we probably want a struct to represent types, so we can annotate it with links later on if needed. (because type can be e.g. `mytemplate<mytype>`). This wouldn't matter except we should probably reuse it...


================
Comment at: clang-tools-extra/clangd/XRefs.h:57
+  /// Includes all the qualifiers.
+  std::string Type;
+  /// Empty for non-functions. First element is the return type.
----------------
sammccall wrote:
> I think we probably want a struct to represent types, so we can annotate it with links later on if needed. (because type can be e.g. `mytemplate<mytype>`). This wouldn't matter except we should probably reuse it...
we may want ReturnType too (unless you want to overload Type for that - I'd suggest against it because of e.g. lambdas)


================
Comment at: clang-tools-extra/clangd/XRefs.h:59
+  /// Empty for non-functions. First element is the return type.
+  std::vector<LocatedSymbol> Signature;
+  std::string Documentation;
----------------
maybe `vector<param>` params, where param is `struct { string name; Type type }`?

We might render as:

***
**Parameters**:
  - **value**: `std::string`
  -  **size**: `int` (default = `0`)


================
Comment at: clang-tools-extra/clangd/XRefs.h:63
+  std::string Definition;
+  std::string TemplateArgs;
+};
----------------
TemplateArgs might want to follow the same structure as params?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61497