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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 6 inline comments 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:
> 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?


================
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);
----------------
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.


================
Comment at: clang-tools-extra/clangd/Protocol.cpp:317
+          for (const auto &Format : *DocumentationFormat) {
+            if (fromJSON(Format, K)) {
+              R.CompletionDocumentationFormat = K;
----------------
kadircet wrote:
> why not just `if (fromJson(Format, R.CompletionDocFormat)` ?
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...)


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:835
       /* Multi-line
          block comment
       */
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79157