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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

(I only got about halfway through the implementation - it's missing a lot of comments I think ;-))



================
Comment at: clang-tools-extra/clangd/XRefs.cpp:539
 
-/// Generate a \p Hover object given the declaration \p D.
-static Hover getHoverContents(const Decl *D) {
-  Hover H;
-  llvm::Optional<std::string> NamedScope = getScopeName(D);
+static QualType getDeclType(const Decl *D) {
+  if (const TypedefNameDecl *TDD = dyn_cast<TypedefNameDecl>(D))
----------------
I think this is basically ASTContext::getTypeDeclType()?


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:549
 
-  // Generate the "Declared in" section.
-  if (NamedScope) {
-    assert(!NamedScope->empty());
+static llvm::Optional<Location> getDeclLoc(const SourceLocation &SL,
+                                           ASTContext &AST) {
----------------
what does this have to do with decls?


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:563
 
-    H.contents.value += "Declared in ";
-    H.contents.value += *NamedScope;
-    H.contents.value += "\n\n";
+static std::string getDefinitionLine(const Decl *D) {
+  std::string Definition;
----------------
printDefinition?


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:574
 
-  // We want to include the template in the Hover.
-  if (TemplateDecl *TD = D->getDescribedTemplate())
-    D = TD;
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
+                              const std::vector<HoverInfo::Param> &Params) {
----------------
I don't think it's reasonable to define this private helper as an overload of operator<<.
Make it a function, or inline it? (I think used only once)


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1207
+                              const HoverInfo::Param &P) {
+  OS << P.T << ' ' << P.Name;
+  if (!P.Default.empty())
----------------
avoid emitting the space if T/name are empty?


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1214
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const HoverInfo &HI) {
+  OS << HI.Definition << '\n';
+  OS << HI.Documentation << '\n';
----------------
This doesn't seem sufficiently self-documenting.


================
Comment at: clang-tools-extra/clangd/XRefs.h:52
+struct HoverInfo {
+  LocatedSymbol Sym;
+  /// Name of the context containing the symbol.
----------------
kadircet wrote:
> sammccall wrote:
> > I'm not sure about reuse of LocatedSymbol - do we want to commit to returning decl/def locations?
> > Name might be enough here.
> It might be nice to provide editors with enough info to jump to definition(it was brought up during last meeting). But happy to reduce it to just name.
(this is not done I think - LocatedSymbol is still here)


================
Comment at: clang-tools-extra/clangd/XRefs.h:73
+  llvm::Optional<std::vector<Param>> Parameters;
+  llvm::Optional<std::vector<Param>> TemplateParameters;
+
----------------
ilya-biryukov wrote:
> kadircet wrote:
> > ilya-biryukov wrote:
> > > What does `Type` mean for non-type and template template parameters?
> > > Could you add a comment?
> > added comment for template template parameters.
> > 
> > For non-type template params, isn't it clear that this will hold the type of that param? e.g `template <C<int>... X>` -> `C<int>...`
> Ah, sure, sorry, I meant the type parameters :-)
I get the curiosity, but this is too much text, and only covers template parameters which are not the most important case.

Can we just say something like `The pretty-printed parameter type, e.g. "int", or "typename" (in TemplateParameters)`?

We should do something sensible for template template parameters, but it's not important or non-obvious enough to document.


================
Comment at: clang-tools-extra/clangd/XRefs.h:52
 
+struct HoverInfo {
+  using Type = std::string;
----------------
This needs docs :-)


================
Comment at: clang-tools-extra/clangd/XRefs.h:54
+  using Type = std::string;
+  struct Param {
+    // For template template params it holds the whole template decl, e.g,
----------------
one line doc for this struct?


================
Comment at: clang-tools-extra/clangd/XRefs.h:58
+    // For type-template params will contain "typename" or "class".
+    Type T;
+    std::string Name;
----------------
needs a real name
mention the no-type case (macros)


================
Comment at: clang-tools-extra/clangd/XRefs.h:59
+    Type T;
+    std::string Name;
+    std::string Default;
----------------
mention the unnamed case


================
Comment at: clang-tools-extra/clangd/XRefs.h:60
+    std::string Name;
+    std::string Default;
+  };
----------------
mention the no-default case


================
Comment at: clang-tools-extra/clangd/XRefs.h:64
+  LocatedSymbol Sym;
+  /// Fully qualiffied name for the scope containing the Sym.
+  std::string Scope;
----------------
qualiffied -> qualified


================
Comment at: clang-tools-extra/clangd/XRefs.h:65
+  /// Fully qualiffied name for the scope containing the Sym.
+  std::string Scope;
+  std::string ParentScope;
----------------
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)


================
Comment at: clang-tools-extra/clangd/XRefs.h:69
+  std::string Documentation;
+  /// Line containing the definition of the symbol.
+  std::string Definition;
----------------
This sounds like it's a source location, (e.g. "file.cc:42:7")
But I think it's rather source code?


================
Comment at: clang-tools-extra/clangd/XRefs.h:72
+
+  /// T and ReturnType are mutually exclusive.
+  llvm::Optional<Type> T;
----------------
I'm not sure this is significant or should even be true (e.g. if T is a function reference?)

Could we explain the relationship between the different fields here instead, maybe briefly with examples? (A function will have return type and parameters set, etc)


================
Comment at: clang-tools-extra/clangd/XRefs.h:73
+  /// T and ReturnType are mutually exclusive.
+  llvm::Optional<Type> T;
+  llvm::Optional<Type> ReturnType;
----------------
T needs a real name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61497