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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
