ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LGTM. Thanks!
================
Comment at: clangd/ClangdLSPServer.cpp:816
+ // If the client supports Markdown, convert from plaintext here.
+ if (*H && HoverSupportsMarkdown) {
+ (*H)->contents.kind = MarkupKind::Markdown;
----------------
malaperle wrote:
> ilya-biryukov wrote:
> > malaperle wrote:
> > > I don't know if you meant plain text as non-language-specific markdown or
> > > no markdown at all. In VSCode, non-markdown for multiline macros looks
> > > bad because the newlines are ignored.
> > Did not know about that, thanks for pointing it out.
> > It does not ignore double newlines though (that's what we use in
> > declarations). I suspect it treats plaintext as markdown, but can't be sure.
> >
> > In any case, converting to a markdown code block here looks incredibly
> > hacky and shaky.
> > Could we use double-newlines for line breaks in our implementation instead?
> >
> > This aligns with what we did before this patch for declarations.
> > If that doesn't work, breaking the multi-line macros in VSCode looks ok,
> > this really feels like a bug in VSCode.
> > In any case, converting to a markdown code block here looks incredibly
> > hacky and shaky.
>
> I'm not sure why? ClangdLSPServer knows about client capabilities so it made
> sense to me to convert it there. Either way, it sounds like I should just
> remove "HoverSupportsMarkdown" altogether if we're not going to use Markdown.
>
> > Could we use double-newlines for line breaks in our implementation instead?
>
> It looks odd. When I double them in the hover contents, the lines are
> displayed as doubled and some extra backslashes are added on all non-empty
> lines except the first line. Single new lines and correct backslashes are
> displayed correctly when Markdown is used. We can just display multiline line
> macros as one line until we want to handle Markdown (which is how exactly?).
> I'm not sure why? ClangdLSPServer knows about client capabilities so it made
> sense to me to convert it there. Either way, it sounds like I should just
> remove "HoverSupportsMarkdown" altogether if we're not going to use Markdown.
Don't get me wrong, adding markdown support seems nice, but let's do it as a
separate patch. Here's a few thoughts on this.
Currently ClangdServer already already returns a hover with proper kind and
contents, so it is ClangdServer's responsibility to produce markdown or
plaintext as of today.
We could consider moving the responsibility to decide whether we should return
markdown or plaintext to LSPServer, but then we should also change the types of
values ClangdServer returns (to a structured representation that would produce
plaintext or markdown, depending on the client settings).
Assuming the result is a code block looks wrong, we do return plain text bits
like "declared in namespace std" as a result of hover.
> It looks odd. When I double them in the hover contents, the lines are
> displayed as doubled and some extra backslashes are added on all non-empty
> lines except the first line. Single new lines and correct backslashes are
> displayed correctly when Markdown is used. We can just display multiline line
> macros as one line until we want to handle Markdown (which is how exactly?).
Totally on board with this, multiline marcos are not exactly rare, but we could
live with this oddity in VSCode for some time.
Adding basic markdown support shouldn't be too hard, but let's block this patch
on it.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55250/new/
https://reviews.llvm.org/D55250
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
