[clangd] Render doc-comment code spans with `backticks` in plaintext mode

[kadircet]

clang-tools-extra/clangd/FormattedString.cpp

365

should we rather use renderInlineBlock here ? because in presence of backticks inside the C.Contents it might become confusing e.g:

this is`foo(`x`)`

this would become:

this is `foo(``x``)`

and instead of keeping a marker maybe just:

if (Preserve && ..)
 OS << Sep << "`" << C.Contents << "`";
else
  OS << Sep << C.Contents;

[sammccall]

clang-tools-extra/clangd/FormattedString.cpp

365

rather use renderInlineBlock

I don't think humans deal with escaping better than with ambiguity - those two examples seem equally confusing to me :-\

I'd suggest better is to pick another marker that's unused in the chunk:

this is 'foo(`x`)'

Happy to do that in this patch if you like it.
instead of keeping a marker maybe just:

instead of keeping a marker maybe just

Happy to do that for now if you like, but I don't think it's much clearer. I factored it this way because I think the marker is likely to have more options in future (bold spans, avoiding conflicts) and wanted to hint at that.

[kadircet]

LGTM thanks!

clang-tools-extra/clangd/FormattedString.cpp

365

I don't think humans deal with escaping better than with ambiguity - those two examples seem equally confusing to me :-\
I'd suggest better is to pick another marker that's unused in the chunk:
this is 'foo(x)'
Happy to do that in this patch if you like it.

Yeah that looks like a better approach, feel free to leave a fixme as well. As I believe this should be a rather rare occurrence and we can take care of it once it becomes a real issue or someone has time.

instead of keeping a marker maybe just

Happy to do that for now if you like, but I don't think it's much clearer. I factored it this way because I think the marker is likely to have more options in future (bold spans, avoiding conflicts) and wanted to hint at that.

OK makes sense let's keep it that way.

clang-tools-extra/clangd/FormattedString.h

 public:
   void renderMarkdown(llvm::raw_ostream &OS) const override;
   void renderPlainText(llvm::raw_ostream &OS) const override;
 
   /// Append plain text to the end of the string.
   Paragraph &appendText(llvm::StringRef Text);
 
   /// Append inline code, this translates to the ` block in markdown.
-  Paragraph &appendCode(llvm::StringRef Code);
+  /// \p Preserve indicates the code span must be apparent even in plaintext.
+  Paragraph &appendCode(llvm::StringRef Code, bool Preserve = false);
 
 private:
   struct Chunk {
     enum {
       PlainText,
       InlineCode,
     } Kind = PlainText;
+    // Preserve chunk markers in plaintext.
+    bool Preserve = false;
     std::string Contents;
     /// Language for code block chunks. Ignored for other chunks.
     std::string Language;
   };
   std::vector<Chunk> Chunks;
 };
 
 /// Represents a sequence of one or more documents. Knows how to print them in a

clang-tools-extra/clangd/FormattedString.cpp

void Paragraph::renderMarkdown(llvm::raw_ostream &OS) const {
   // Therefore it only has a single linebreak afterwards.
   // VSCode requires two spaces at the end of line to start a new one.
   OS << "  \n";
 }
 
 void Paragraph::renderPlainText(llvm::raw_ostream &OS) const {
   llvm::StringRef Sep = "";
   for (auto &C : Chunks) {
-    OS << Sep << C.Contents;
+    llvm::StringRef Marker = "";
+    if (C.Preserve && C.Kind == Chunk::InlineCode)
+      Marker = "`";
+    OS << Sep << Marker << C.Contents << Marker;
     Sep = " ";
   }
   OS << '\n';
 }
 
 void BulletList::renderMarkdown(llvm::raw_ostream &OS) const {
   for (auto &D : Items) {
     // Instead of doing this we might prefer passing Indent to children to get
if (Norm.empty())
     return *this;
   Chunks.emplace_back();
   Chunk &C = Chunks.back();
   C.Contents = std::move(Norm);
   C.Kind = Chunk::PlainText;
   return *this;
 }
 
-Paragraph &Paragraph::appendCode(llvm::StringRef Code) {
+Paragraph &Paragraph::appendCode(llvm::StringRef Code, bool Preserve) {
   std::string Norm = canonicalizeSpaces(std::move(Code));
   if (Norm.empty())
     return *this;
   Chunks.emplace_back();
   Chunk &C = Chunks.back();
   C.Contents = std::move(Norm);
   C.Kind = Chunk::InlineCode;
+  C.Preserve = Preserve;
   return *this;
 }
 
 class Document &BulletList::addItem() {
   Items.emplace_back();
   return Items.back();
 }
 

clang-tools-extra/clangd/Hover.cpp

 
 void parseDocumentationLine(llvm::StringRef Line, markup::Paragraph &Out) {
   // Probably this is appendText(Line), but scan for something interesting.
   for (unsigned I = 0; I < Line.size(); ++I) {
     switch (Line[I]) {
       case '`':
         if (auto Range = getBacktickQuoteRange(Line, I)) {
           Out.appendText(Line.substr(0, I));
-          Out.appendCode(Range->trim("`"));
+          Out.appendCode(Range->trim("`"), /*Preserve=*/true);
           return parseDocumentationLine(Line.substr(I+Range->size()), Out);
         }
         break;
     }
   }
   Out.appendText(Line);
 }
 

clang-tools-extra/clangd/unittests/HoverTests.cpp

llvm::StringRef ExpectedRenderPlainText;
                    "foo\nbar",
                    "foo bar",
                    "foo bar",
                },
                {
                    // FIXME: we insert spaces between code and text chunk.
                    "Tests primality of `p`.",
                    "Tests primality of `p` .",
-                   "Tests primality of p .",
+                   "Tests primality of `p` .",
                },
                {
                    "'`' should not occur in `Code`",
                    "'\\`' should not occur in `Code`",
-                   "'`' should not occur in Code",
+                   "'`' should not occur in `Code`",
                },
                {
                    "`not\nparsed`",
                    "\\`not parsed\\`",
                    "`not parsed`",
                }};
 
   for (const auto &C : Cases) {