[PATCH] D79157: [clangd] Render code complete documentation as plaintext/markdown.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

LGTM thanks!



================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:377
+      auto SetDoc = [&](llvm::StringRef Doc) {
+        if (!Doc.trim().empty()) {
+          Completion.Documentation.emplace();
----------------
sammccall wrote:
> kadircet wrote:
> > why do we ignore whitespace only comments now ?
> > 
> > nit: early exit;
> > ```
> > if(Doc.trim().empty())
> >   return;
> > ...
> > ```
> > 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?
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.


================
Comment at: clang-tools-extra/clangd/Hover.h:83
 // Try to infer structure of a documentation comment (e.g. line breaks).
+// FIXME: move to another file so CodeComplete doesn't depend on Hover.
 void parseDocumentation(llvm::StringRef Input, markup::Document &Output);
----------------
sammccall wrote:
> kadircet wrote:
> > yeah was about to say that :D what about formattedstring.h with a different name like `parseRaw` ?
> 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.
SGTM.


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:835
       /* Multi-line
          block comment
       */
----------------
sammccall wrote:
> kadircet wrote:
> > 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
> 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.
oh i see. i must be remembering the discussion, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79157/new/

https://reviews.llvm.org/D79157