This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Render code complete documentation as plaintext/markdown.
ClosedPublic

Authored by sammccall on Apr 30 2020, 1:49 AM.

Details

Summary

Structure is parsed from the raw comment using the existing heuristics used
for hover.

Diff Detail

Event Timeline

sammccall created this revision.Apr 30 2020, 1:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2020, 1:49 AM
kadircet marked an inline comment as done.Apr 30 2020, 2:40 AM

thanks, mostly LG apart from dropping of white-space only comments.

clang-tools-extra/clangd/CodeComplete.cpp
377

why do we ignore whitespace only comments now ?

nit: early exit;

if(Doc.trim().empty())
  return;
...
clang-tools-extra/clangd/Hover.h
83

yeah was about to say that :D what about formattedstring.h with a different name like parseRaw ?

clang-tools-extra/clangd/Protocol.cpp
317

why not just if (fromJson(Format, R.CompletionDocFormat) ?

347

i know it is not related to this change, but you've touched it :P so same as above, maybe just fromJson(Format, R.HoverContentFormat)

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
835

no action needed here(apart from maybe filing a bug), but this one looks like a hard-break to me, as the line below is longer in width. why is parseDocumentation not preserving it?

It might also be the case that i am just wrong, as I didn't check the line break heuristics :D

sammccall marked 6 inline comments as done.Apr 30 2020, 4:21 AM
sammccall added inline comments.
clang-tools-extra/clangd/CodeComplete.cpp
377

why do we ignore whitespace only comments now ?

Having the empty-check at all is because the type changed from string-or-empty to document-or-null.

I threw the trim in "while there" because ws-only seems "functionally-empty" - happy to revert on the basis of "unrelated change" but is there an actual reason to include these? Or just the wrong layer?

clang-tools-extra/clangd/Hover.h
83

I'm iffy on the conceptual dependency - FormattedString has a clear responsibility and is a good candidate for moving to Support.

The actual interface and deps are fine today, but it's going to get more complicated when (if?) we want to have it fish out param docs and return value docs for attaching to hover.

Anyway it's definitely an option, but I wanted to do it separately because it's a refactoring change and also not totally obvious where the right place is.

clang-tools-extra/clangd/Protocol.cpp
317

Yeah, that's better.

(There's no contract that fromJSON leaves the output unchanged when it returns false, and sometimes it doesn't e.g. for objects constructed in place. But it's an enum, what's the worst that could happen...)

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
835

This isn't implemented, though was discussed. Not sure how much it adds in practice when we already have the punctuation rule.

Added a comment to isHardLineBreakAfter in Hover.cpp.

sammccall updated this revision to Diff 261185.Apr 30 2020, 4:21 AM
sammccall marked an inline comment as done.

address comments

kadircet accepted this revision.Apr 30 2020, 7:04 AM

LGTM thanks!

clang-tools-extra/clangd/CodeComplete.cpp
377

the trimming behaviour is already performed by FormattedString in appendText, as we cannonicalizeSpaces and ignore the chunk if it is whitespace only during the process. so i was confused to see it here too.

Previous code would accept and surface white-space only comment as-is. Now we'll ignore such comments (other options is displaying an empty comment).

I suppose doing the check below in LSP conversion might be more clear, since from code completion's perspective we've actually found a comment unless it is empty.

clang-tools-extra/clangd/Hover.h
83

SGTM.

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
835

oh i see. i must be remembering the discussion, thanks!

This revision is now accepted and ready to land.Apr 30 2020, 7:04 AM
sammccall marked an inline comment as done.Apr 30 2020, 9:57 AM
sammccall added inline comments.
clang-tools-extra/clangd/CodeComplete.cpp
377

the trimming behaviour is already performed by FormattedString in appendText, as we cannonicalizeSpaces and ignore the chunk if it is whitespace only during the process. so i was confused to see it here too.

Yeah, this is too late - we want to avoid creating the markup::Document at all.

Nevertheless, reverted the change, it's basically unrelated and I don't have a test.

This revision was automatically updated to reflect the committed changes.