[PATCH] D55250: [clangd] Enhance macro hover to see full definition

[Ilya Biryukov via Phabricator via cfe-commits]

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