sammccall added a comment.
This looks pretty nice to me! Sorry for the wait.
Adding @kadircet as hover-guru-in-waiting.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:814
+ return Reply(llvm::None);
+ // FIXME: render as markdown if client supports it.
+ Hover R;
----------------
(I'd like to see that in this patch if possible, it shouldn't be much work)
================
Comment at: clang-tools-extra/clangd/ClangdServer.h:186
void findHover(PathRef File, Position Pos,
- Callback<llvm::Optional<Hover>> CB);
+ Callback<llvm::Optional<FormattedString>> CB);
----------------
Not sure about switching from `Hover` to `FormattedString` as return type here:
LSP hover struct contains a range field (what are the bounds of the hovered
thing?) that we may want to populate that doesn't fit in FormattedString.
I'd suggest the function in XRefs.h should return `FormattedString` (and later
maybe `pair<FormattedString, Range>`), and the `ClangdServer` version should
continue to provide the rendered `Hover`.
(We'll eventually want ClangdServer to return some HoverInfo struct with
structured information, as we want embedding clients to be able to render it
other ways, and maybe use it to provide extension methods like YCM's `GetType`.
But no need to touch that in this patch, we'll end up producing HoverInfo ->
FormattedString -> LSP Hover, I guess)
================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:41
+
+void FormattedString::appendCodeBlock(std::string Code) {
+ Chunk C;
----------------
you may want to take the language name, and default it to either cpp or nothing.
Including it in the triple-backtick block can trigger syntax highlighting, and
editors are probably somewhat likely to actually implement this.
================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:63
+ case ChunkKind::InlineCodeBlock:
+ R += " `";
+ R += escapeBackticks(C.Contents);
----------------
why do we add surrounding spaces if we don't do the same for text chunks?
================
Comment at: clang-tools-extra/clangd/FormattedString.h:1
+//===--- FormattedString.h ----------------------------------*-
C++-*------===//
+//
----------------
This will need tests as you note, which shouldn't be hard.
It's not terribly complicated, but it's the sort of thing that may accumulate
special cases.
================
Comment at: clang-tools-extra/clangd/FormattedString.h:27
+ /// Append plain text to the end of the string.
+ void appendText(std::string Text);
+ /// Append a block of C++ code. This translates to a ``` block in markdown.
----------------
I guess this will get an optional parameter to control the style (bold, italic
etc)?
================
Comment at: clang-tools-extra/clangd/FormattedString.h:31
+ /// newlines.
+ void appendCodeBlock(std::string Code);
+ /// Append an inline block of C++ code. This translates to the ` block in
----------------
Having various methods that take strings may struggle if we want a lot of
composability (e.g. a bullet list whose bullet whose third word is a
hyperlink/bold).
(I'm not sure whether this is going to be a real problem, just thinking out
loud here)
One alternative extensibility would be to make "containers" methods taking a
lambda that renders the body.
e.g.
```
FormattedString S;
S << "std::";
S.bold([&} S << "swap" };
S.list(FormattedString::Numbered, [&] {
S.item([&] { S << "sometimes you get the wrong overload"; });
S.item([&] {
S << "watch out for ";
S.link("https://en.cppreference.com/w/cpp/language/adl";, [&] { S << "ADL";
});
});
});
S.codeBlock([&] S << "Here is an example"; });
```
Not sure if this is really better, I just have it on my mind because I ended up
using it for the `JSON.h` streaming output. We can probably bolt it on later if
necessary.
================
Comment at: clang-tools-extra/clangd/FormattedString.h:39
+ std::string renderAsPlainText() const;
+
+private:
----------------
I think we want renderForDebugging() or so which would print the chunks
explicitly, a la CodeCompletionString.
This is useful for actual debugging, and for testing methods that return
FormattedString (as opposed to the FormattedString->markdown translation)
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:528
+ R.appendText("Declared in ");
+ R.appendText(*NamedScope);
+ R.appendText("\n");
----------------
should this be an inline code block?
================
Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1157
EXPECT_NE("", Test.ExpectedHover) << Test.Input;
- EXPECT_EQ(H->contents.value, Test.ExpectedHover.str()) << Test.Input;
+ EXPECT_EQ(H->renderAsPlainText(), Test.ExpectedHover.str()) <<
Test.Input;
} else
----------------
When this lands for real, I think we should assert on the renderForDebugging()
output or similar.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58547/new/
https://reviews.llvm.org/D58547
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
