[PATCH] D77456: [clangd] Parse `foo` in documentation comments and render as code.

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG30d17d885283: [clangd] Parse `foo` in documentation comments 
and render as code. (authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D77456?vs=255010=261063#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77456

Files:
  clang-tools-extra/clangd/FormattedString.cpp
  clang-tools-extra/clangd/FormattedString.h
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -1958,7 +1958,7 @@
   }
 }
 
-TEST(Hover, DocCommentLineBreakConversion) {
+TEST(Hover, ParseDocumentation) {
   struct Case {
 llvm::StringRef Documentation;
 llvm::StringRef ExpectedRenderMarkdown;
@@ -2017,6 +2017,22 @@
"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 .",
+   },
+   {
+   "'`' should not occur in `Code`",
+   "'\\`' should not occur in `Code`",
+   "'`' should not occur in Code",
+   },
+   {
+   "`not\nparsed`",
+   "\\`not parsed\\`",
+   "`not parsed`",
}};
 
   for (const auto  : Cases) {
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -772,7 +772,7 @@
   // https://github.com/microsoft/vscode/issues/88417 for details.
   markup::Paragraph  = Output.addHeading(3);
   if (Kind != index::SymbolKind::Unknown)
-Header.appendText(std::string(index::getSymbolKindString(Kind)));
+Header.appendText(index::getSymbolKindString(Kind));
   assert(!Name.empty() && "hover triggered on a nameless symbol");
   Header.appendCode(Name);
 
@@ -809,10 +809,11 @@
 
   if (Offset)
 Output.addParagraph().appendText(
-llvm::formatv("Offset: {0} byte{1}", *Offset, *Offset == 1 ? "" : "s"));
+llvm::formatv("Offset: {0} byte{1}", *Offset, *Offset == 1 ? "" : "s")
+.str());
   if (Size)
 Output.addParagraph().appendText(
-llvm::formatv("Size: {0} byte{1}", *Size, *Size == 1 ? "" : "s"));
+llvm::formatv("Size: {0} byte{1}", *Size, *Size == 1 ? "" : "s").str());
 
   if (!Documentation.empty())
 parseDocumentation(Documentation, Output);
@@ -838,6 +839,52 @@
   return Output;
 }
 
+// If the backtick at `Offset` starts a probable quoted range, return the range
+// (including the quotes).
+llvm::Optional getBacktickQuoteRange(llvm::StringRef Line,
+  unsigned Offset) {
+  assert(Line[Offset] == '`');
+
+  // The open-quote is usually preceded by whitespace.
+  llvm::StringRef Prefix = Line.substr(0, Offset);
+  constexpr llvm::StringLiteral BeforeStartChars = " \t(=";
+  if (!Prefix.empty() && !BeforeStartChars.contains(Prefix.back()))
+return llvm::None;
+
+  // The quoted string must be nonempty and usually has no leading/trailing ws.
+  auto Next = Line.find('`', Offset + 1);
+  if (Next == llvm::StringRef::npos)
+return llvm::None;
+  llvm::StringRef Contents = Line.slice(Offset + 1, Next);
+  if (Contents.empty() || isWhitespace(Contents.front()) ||
+  isWhitespace(Contents.back()))
+return llvm::None;
+
+  // The close-quote is usually followed by whitespace or punctuation.
+  llvm::StringRef Suffix = Line.substr(Next + 1);
+  constexpr llvm::StringLiteral AfterEndChars = " \t)=.,;:";
+  if (!Suffix.empty() && !AfterEndChars.contains(Suffix.front()))
+return llvm::None;
+
+  return Line.slice(Offset, Next+1);
+}
+
+void parseDocumentationLine(llvm::StringRef Line, markup::Paragraph ) {
+  // 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("`"));
+  return parseDocumentationLine(Line.substr(I+Range->size()), Out);
+}
+break;
+}
+  }
+  Out.appendText(Line);
+}
+
 void parseDocumentation(llvm::StringRef Input, markup::Document ) {
   std::vector ParagraphLines;
   auto FlushParagraph = [&] {
@@ -845,7 +892,7 @@
   return;
 auto  = Output.addParagraph();
 for 

[PATCH] D77456: [clangd] Parse `foo` in documentation comments and render as code.

[Sam McCall via Phabricator via cfe-commits]

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

Discussed offline, will follow up with fix for spaces and preserving backticks 
in text.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77456



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77456: [clangd] Parse `foo` in documentation comments and render as code.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D77456#1991484 , @sammccall wrote:

> No, I'm complaining about the space before the period in
>
>   Tests primality of `p` .
>
>
> and the plaintext rendering too
>
>   Tests primality of p .
>


Ah I see, yes this looks annoying, and it is only because the next block starts 
with a punctuation :/

> Two main objections to the idea of "raw":
> 
> - we're going to emit arbitrary, potentially malformed markdown into the 
> markdown stream, which can have arbitrary effects/glitches. I'd rather the 
> emitter always emits valid markup and thus can't lose track of the context.
> - this assumes the input is markdown. I want `\c foo` to also render as 
> code-font `foo` in markdown and as backtick-foo in plaintext. So what do we 
> do there, generate markdown as a string and then emit it as a raw chunk? What 
> a mess.
> 
>   I really do think what we want is a chunk with semantics "emphasized code" 
> that renders as a code span in markdown and as backtick-delimited text in 
> plaintext. Thus the proposal to put an emphasis bit on the code chunk. WDYT?

Sorry I wasn't clear on my comment. I was also suggesting putting some markdown 
generated by us while parsing the documentation into this raw field, which 
would be rendered as-is. In case we would like to perform this for other 
markers like `*`.
So going with an emphasis-only solution is also OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77456



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77456: [clangd] Parse `foo` in documentation comments and render as code.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D77456#1961374 , @kadircet wrote:

> LGTM, thanks!
>
> Regarding spaces between code and text chunks, are you suggesting we should 
> print:
>
>   Tests primality of`p`
>


No, I'm complaining about the space before the period in

  Tests primality of `p` .

and the plaintext rendering too

  Tests primality of p .



> So having a raw paragraph chunk(in addition to plaintext and inline code). 
> might be a middle ground here. Plaintext renderers will keep showing the 
> documentation as it is written in the source code, including backticks, 
> whereas markdown renderers would display a more rich text. WDYT?

Two main objections to the idea of "raw":

- we're going to emit arbitrary, potentially malformed markdown into the 
markdown stream, which can have arbitrary effects/glitches. I'd rather the 
emitter always emits valid markup and thus can't lose track of the context.
- this assumes the input is markdown. I want `\c foo` to also render as 
code-font `foo` in markdown and as backtick-foo in plaintext. So what do we do 
there, generate markdown as a string and then emit it as a raw chunk? What a 
mess.

I really do think what we want is a chunk with semantics "emphasized code" that 
renders as a code span in markdown and as backtick-delimited text in plaintext. 
Thus the proposal to put an emphasis bit on the code chunk. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77456



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77456: [clangd] Parse `foo` in documentation comments and render as code.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

> But I'm wondering how good an idea this is for plaintext: "Returns 'foo' on 
> failure" is better than "Returns foo on failure", right? (With backticks 
> instead of single quotes, phab is eating my formatting)

ah sorry missed that one.

That's actually a good point, I didn't dwell on it too much because I don't 
think having backticks in plaintext is that useful. But thinking about it again 
some people might find it useful especially in long
documentations to figure out which parts of it to focus on. So this might be a 
regression for them, it would be nice to have some feedback mechanism for 
driving such decisions. So far it has been just
the reviewer and the author of the patch :D

So having a `raw` paragraph chunk(in addition to plaintext and inline code). 
might be a middle ground here.  Plaintext renderers will keep showing the 
documentation as it is written in the source code,
including backticks, whereas markdown renderers would display a more `rich` 
text. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77456



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77456: [clangd] Parse `foo` in documentation comments and render as code.

[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!

Regarding spaces between code and text chunks, are you suggesting we should 
print:

  Tests primality of`p`

if so, i do believe having a space before the backtick vs not having it is two 
totally different rendering decisions. And I think the former is more common, 
why do you think we should not put that space?
I suppose we can modify the spacing logic for markdown to only put a separator 
between same chunk types, but I don't think that'll look any better especially 
in plugins like coc.nvim.




Comment at: clang-tools-extra/clangd/Hover.cpp:840
+void parseDocumentationLine(llvm::StringRef Line, markup::Paragraph ) {
+  // Probably this is appendText(Input), but scan for something interesting.
+  for (unsigned I = 0; I < Line.size(); ++I) {

nit: s/Input/Line/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77456



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77456: [clangd] Parse `foo` in documentation comments and render as code.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Some saturday morning procrastination.

But I'm wondering how good an idea this is for plaintext: "Returns 'foo' on 
failure" is better than "Returns foo on failure", right? (With backticks 
instead of single quotes, phab is eating my formatting)
Maybe we should have an Emphasis flag or something on plaintext to preserve the 
quotes? (We don't want to include them e.g. in the return type chunks)

Also, realized our model for whitespace around paragraph chunks isn't ideal 
after all :-(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77456



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77456: [clangd] Parse `foo` in documentation comments and render as code.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a project: clang.
sammccall added a comment.

Some saturday morning procrastination.

But I'm wondering how good an idea this is for plaintext: "Returns 'foo' on 
failure" is better than "Returns foo on failure", right? (With backticks 
instead of single quotes, phab is eating my formatting)
Maybe we should have an Emphasis flag or something on plaintext to preserve the 
quotes? (We don't want to include them e.g. in the return type chunks)

Also, realized our model for whitespace around paragraph chunks isn't ideal 
after all :-(


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77456

Files:
  clang-tools-extra/clangd/FormattedString.cpp
  clang-tools-extra/clangd/FormattedString.h
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -1935,7 +1935,7 @@
   }
 }
 
-TEST(Hover, DocCommentLineBreakConversion) {
+TEST(Hover, ParseDocumentation) {
   struct Case {
 llvm::StringRef Documentation;
 llvm::StringRef ExpectedRenderMarkdown;
@@ -1994,6 +1994,22 @@
"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 .",
+   },
+   {
+   "'`' should not occur in `Code`",
+   "'\\`' should not occur in `Code`",
+   "'`' should not occur in Code",
+   },
+   {
+   "`not\nparsed`",
+   "\\`not parsed\\`",
+   "`not parsed`",
}};
 
   for (const auto  : Cases) {
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -747,7 +747,7 @@
   // https://github.com/microsoft/vscode/issues/88417 for details.
   markup::Paragraph  = Output.addHeading(3);
   if (Kind != index::SymbolKind::Unknown)
-Header.appendText(std::string(index::getSymbolKindString(Kind)));
+Header.appendText(index::getSymbolKindString(Kind));
   assert(!Name.empty() && "hover triggered on a nameless symbol");
   Header.appendCode(Name);
 
@@ -806,6 +806,53 @@
   return Output;
 }
 
+// If the backtick at `Offset` starts a probable quoted range, return the range
+// (including the quotes).
+llvm::Optional getBacktickQuoteRange(llvm::StringRef Line,
+  unsigned Offset) {
+  assert(Line[Offset] == '`');
+
+  // The open-quote is usually preceded by whitespace.
+  llvm::StringRef Prefix = Line.substr(0, Offset);
+  constexpr llvm::StringLiteral BeforeStartChars = " \t(=";
+  if (!Prefix.empty() && !BeforeStartChars.contains(Prefix.back()))
+return llvm::None;
+
+  // The quoted string must be nonempty and usually has no leading/trailing ws.
+  auto Next = Line.find('`', Offset + 1);
+  if (Next == llvm::StringRef::npos)
+return llvm::None;
+  llvm::StringRef Contents = Line.slice(Offset + 1, Next);
+  if (Contents.empty() || isWhitespace(Contents.front()) ||
+  isWhitespace(Contents.back()))
+return llvm::None;
+
+  // The close-quote is usually followed by whitespace or punctuation.
+  llvm::StringRef Suffix = Line.substr(Next + 1);
+  constexpr llvm::StringLiteral AfterEndChars = " \t)=.,;:";
+  if (!Suffix.empty() && !AfterEndChars.contains(Suffix.front()))
+return llvm::None;
+
+  return Line.slice(Offset, Next+1);
+}
+
+void parseDocumentationLine(llvm::StringRef Line, markup::Paragraph ) {
+  // Probably this is appendText(Input), 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("`"));
+  return parseDocumentationLine(Line.substr(I+Range->size()), Out);
+}
+break;
+
+}
+  }
+  Out.appendText(Line);
+}
+
 void parseDocumentation(llvm::StringRef Input, markup::Document ) {
   std::vector ParagraphLines;
   auto FlushParagraph = [&] {
@@ -813,7 +860,7 @@
   return;
 auto  = Output.addParagraph();
 for (llvm::StringRef Line : ParagraphLines)
-  P.appendText(Line.str());
+  parseDocumentationLine(Line, P);
 ParagraphLines.clear();
   };
 
Index: