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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked an inline comment as done.
sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:377
+      auto SetDoc = [&](llvm::StringRef Doc) {
+        if (!Doc.trim().empty()) {
+          Completion.Documentation.emplace();
----------------
kadircet wrote:
> 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.
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79157