[PATCH] D71555: [clangd] Refurbish HoverInfo::present

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Great stuff, let's finally ship it!



================
Comment at: clang-tools-extra/clangd/Hover.cpp:466
+  markup::Paragraph &P = Output.addParagraph();
+  P.appendText(beautify(index::getSymbolKindString(Kind)));
+  if (!Name.empty()) {
----------------
kadircet wrote:
> sammccall wrote:
> > is beautify() actually better here? I actually quite like the lowercase as it reduces the emphasis on the kind vs the name, and the hyphens as it makes it easier to pick out the name.
> > 
> > This is up to you of course.
> I've actually found those hyphens annoying, but don't have any strong feelings towards those. I would land it this way and see how ppl feel about it. (but of course, if you've got any strong feelings we could also do it the other way around)
ok, that's fine. If you don't care much about the case, consider dropping the case transform and just replacing `-` with ` `, it seems less noisy. But up to you, fine to leave as-is.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:469
+    P.appendCode(Name);
+    if (Type && !ReturnType) {
+      P.appendText(":");
----------------
kadircet wrote:
> sammccall wrote:
> > if ReturnType exists and non-void, we could consider `->` or `→` in place of `:`.
> we've got a special `Returns: x` paragraph for function-likes, therefore I wasn't printing anything after the name for functions.
> 
> but thinking about it now, actually it makes sense to drop that line and print something like:
> 
> ```
> Function `foo` -> `int`
> - `bool param1`
> ```
> 
> wdyt?
Yeah, I think that'd be better and more consistent with Type.
(except in cases where the name and/or return type are long, but that's probably equally true with Type/ReturnType)


================
Comment at: clang-tools-extra/clangd/Hover.cpp:520
+    OS << Definition;
+    Output.addCodeBlock(OS.str());
+  }
----------------
kadircet wrote:
> sammccall wrote:
> > If you're going to use comment syntax, shouldn't the comment be part of the code block?
> it is, I am putting everything to stream and then creating a codeblock out of it.
Right, sorry!

nit: OS here seems a little obscure/overkill, could be just
```std::string ScopeComment;
if (...) {
  ScopeComment = "//...\n";
}
Output.addCodeBlock(ScopeComment + Definition);
```


================
Comment at: clang-tools-extra/clangd/Hover.cpp:490
+  markup::Paragraph &Header = Output.addParagraph();
+  Header.appendText(beautify(index::getSymbolKindString(Kind)));
+  if (!Name.empty()) {
----------------
if there's no name, do we want to skip the header entirely?
(I can't think of examples for this case.)


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:1630
+          },
+          R"(Namespace Alias foo)",
+      },
----------------
I do find the uppercase `A` next to the lowercase `foo` here jarring.

This looks like "title case", but in title case the filler words are lowercase and the "important" ones are capitalized, and here we'd have the opposite.

"Namespace alias foo" or "namespace alias foo" both seem fine.


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:1462
+            HI.Kind = index::SymbolKind::NamespaceAlias;
+            HI.Name = "foo";
+          },
----------------
kadircet wrote:
> sammccall wrote:
> > what does this test incrementally to the previous test case?
> it was rather for checking replacement of hyphens.
right - maybe delete the "class foo" test then?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71555