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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

comments on interface again, will take another pass at implementation



================
Comment at: clang-tools-extra/clangd/XRefs.h:65
+  /// Fully qualiffied name for the scope containing the Sym.
+  std::string Scope;
+  std::string ParentScope;
----------------
kadircet wrote:
> sammccall wrote:
> > what's the difference between Scope and ParentScope?
> > Can we give them more obvious names, or a comment?
> > (The current comment doesn't really say anything)
> We've been using Name and Scope to represent two parts of qualified names across the codebase.
> Comment was unfortunately misplaced :/ LMK if you have any other choices for the name
Yes, but here you've got three concepts: name/scope/parentscope (now called containedin?). And this covers lots of cases that our existing Name/Scope doesn't really (there are lots of symbols we don't index).

It wasn't a rhetorical question, I can't suggest better names because I don't know what they are.

My suggestion would be to split into namespace scope/local scope/name, such that Name has no `::`, `NamespaceScope` has exactly the (non-inline) enclosing namespaces, and   `NamespaceScope + LocalScope + Name == QName`.

So for a local in a class member, `NamespaceScope == "clang::clangd""`, `LocalScope == "SymbolCollector::consume::` and `Name = Symbols`. Or just `Name = Symbols` and the others are blank, depending on how we choose to display locals.


================
Comment at: clang-tools-extra/clangd/XRefs.h:53
+/// Contains detailed information about a Symbol. Especially useful when
+/// generating hover responses. It is not serialized so that embedding clients
+/// can choose to serialize it w.r.t their UI capabilities.
----------------
I'm having trouble following the "serialized" sentence. Maybe "It can be rendered as a hover panel, or embedding clients can use the structured information to provide their own UI"?


================
Comment at: clang-tools-extra/clangd/XRefs.h:76
+  /// - None for deduced types, e.g "auto", "decltype" keywords.
+  llvm::Optional<std::string> ContainedIn;
+  SymbolKind Kind;
----------------
This seems overlapping/non-orthogonal with scope. It doesn't let us render a member as `ClassName::method` for instance.
If we really need "In function x::Y" rather than "in x::Y", we could add a member "ImmediateScopeKind" or something. But I'm not sure we do.


================
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;
----------------
nit: just "templates"?

(e.g. if we hover over a call to std::swap, we might be showing the instantiation rather than the declaration?)


================
Comment at: clang-tools-extra/clangd/XRefs.h:93
+  /// Lower to LSP struct.
+  Hover render() const;
+};
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61497