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

[Kadir Cetinkaya via Phabricator via cfe-commits]

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


================
Comment at: clang-tools-extra/clangd/Hover.cpp:393
+// Converts a string of the form `word1-word2-...` into `Word1 Word2 ...`.
+std::string beautify(llvm::StringRef Input) {
+  std::string Res;
----------------
sammccall wrote:
> It's worth noting that an alternative to mechanically transforming would simply be to change the getSymbolKindString(). It's only used in tests (we'd have to update the tests though).
or we can have our own SymbolKind to string conversion. I would be OK with doing that(rather than changing getSymbolKindString) if you believe mechanical conversion isn't enough.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:466
+  markup::Paragraph &P = Output.addParagraph();
+  P.appendText(beautify(index::getSymbolKindString(Kind)));
+  if (!Name.empty()) {
----------------
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)


================
Comment at: clang-tools-extra/clangd/Hover.cpp:469
+    P.appendCode(Name);
+    if (Type && !ReturnType) {
+      P.appendText(":");
----------------
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?


================
Comment at: clang-tools-extra/clangd/Hover.cpp:474
+  } else if (Type) {
+    P.appendCode(*Type);
   }
----------------
sammccall wrote:
> what *is* the case where something has a type but no name?
there's none, it was a leftover from old revisions, nvm.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:513
+      } else if (!NamespaceScope->empty()) {
+        OS << "// namespace " << llvm::StringRef(*NamespaceScope).drop_back(2);
+      } else {
----------------
sammccall wrote:
> Either "// in namespace ..." or "// namespace ... {" or "namespace ... {" ?
going with `In namespace ...`, having a `{` in the end without an enclosing `}` doesn't seem right.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:515
+      } else {
+        OS << "// global namespace";
+      }
----------------
sammccall wrote:
> either "// in global namespace" or nothing at all here?
> 
> (This text is probably annoying for non-C++ projects, or C++ projects that don't use namespaces - I'd be tempted to let the absence of a NS stand for itself)
agreed, dropped that case and put some comments to explain why.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:520
+    OS << Definition;
+    Output.addCodeBlock(OS.str());
+  }
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71555