[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL360151: [clangd] Introduce intermediate representation of 
formatted text (authored by ibiryukov, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D58547?vs=198463=198466#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58547

Files:
  clang-tools-extra/trunk/clangd/CMakeLists.txt
  clang-tools-extra/trunk/clangd/FormattedString.cpp
  clang-tools-extra/trunk/clangd/FormattedString.h
  clang-tools-extra/trunk/clangd/unittests/CMakeLists.txt
  clang-tools-extra/trunk/clangd/unittests/FormattedStringTests.cpp

Index: clang-tools-extra/trunk/clangd/unittests/FormattedStringTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/FormattedStringTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/FormattedStringTests.cpp
@@ -0,0 +1,156 @@
+//===-- FormattedStringTests.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "FormattedString.h"
+#include "clang/Basic/LLVM.h"
+#include "llvm/ADT/StringRef.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TEST(FormattedString, Basic) {
+  FormattedString S;
+  EXPECT_EQ(S.renderAsPlainText(), "");
+  EXPECT_EQ(S.renderAsMarkdown(), "");
+
+  S.appendText("foobar");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar");
+  EXPECT_EQ(S.renderAsMarkdown(), "foobar");
+
+  S = FormattedString();
+  S.appendInlineCode("foobar");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar");
+  EXPECT_EQ(S.renderAsMarkdown(), "`foobar`");
+
+  S = FormattedString();
+  S.appendCodeBlock("foobar");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar");
+  EXPECT_EQ(S.renderAsMarkdown(), "```cpp\n"
+  "foobar\n"
+  "```\n");
+}
+
+TEST(FormattedString, CodeBlocks) {
+  FormattedString S;
+  S.appendCodeBlock("foobar");
+  S.appendCodeBlock("bazqux", "javascript");
+
+  EXPECT_EQ(S.renderAsPlainText(), "foobar\n\n\nbazqux");
+  std::string ExpectedMarkdown = R"md(```cpp
+foobar
+```
+```javascript
+bazqux
+```
+)md";
+  EXPECT_EQ(S.renderAsMarkdown(), ExpectedMarkdown);
+
+  S = FormattedString();
+  S.appendInlineCode("foobar");
+  S.appendInlineCode("bazqux");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar bazqux");
+  EXPECT_EQ(S.renderAsMarkdown(), "`foobar` `bazqux`");
+
+  S = FormattedString();
+  S.appendText("foo");
+  S.appendInlineCode("bar");
+  S.appendText("baz");
+
+  EXPECT_EQ(S.renderAsPlainText(), "foo bar baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "foo`bar`baz");
+}
+
+TEST(FormattedString, Escaping) {
+  // Check some ASCII punctuation
+  FormattedString S;
+  S.appendText("*!`");
+  EXPECT_EQ(S.renderAsMarkdown(), "\\*\\!\\`");
+
+  // Check all ASCII punctuation.
+  S = FormattedString();
+  std::string Punctuation = R"txt(!"#$%&'()*+,-./:;<=>?@[\]^_`{|}~)txt";
+  // Same text, with each character escaped.
+  std::string EscapedPunctuation;
+  EscapedPunctuation.reserve(2 * Punctuation.size());
+  for (char C : Punctuation)
+EscapedPunctuation += std::string("\\") + C;
+  S.appendText(Punctuation);
+  EXPECT_EQ(S.renderAsMarkdown(), EscapedPunctuation);
+
+  // In code blocks we don't need to escape ASCII punctuation.
+  S = FormattedString();
+  S.appendInlineCode("* foo !+ bar * baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "`* foo !+ bar * baz`");
+  S = FormattedString();
+  S.appendCodeBlock("#define FOO\n* foo !+ bar * baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "```cpp\n"
+  "#define FOO\n* foo !+ bar * baz\n"
+  "```\n");
+
+  // But we have to escape the backticks.
+  S = FormattedString();
+  S.appendInlineCode("foo`bar`baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "`foo``bar``baz`");
+
+  S = FormattedString();
+  S.appendCodeBlock("foo`bar`baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "```cpp\n"
+  "foo`bar`baz\n"
+  "```\n");
+
+  // Inline code blocks starting or ending with backticks should add spaces.
+  S = FormattedString();
+  S.appendInlineCode("`foo");
+  EXPECT_EQ(S.renderAsMarkdown(), "` ``foo `");
+  S = FormattedString();
+  S.appendInlineCode("foo`");
+  EXPECT_EQ(S.renderAsMarkdown(), "` foo`` `");
+  S = FormattedString();
+  S.appendInlineCode("`foo`");
+  EXPECT_EQ(S.renderAsMarkdown(), "` ``foo`` `");
+
+  // Should also add extra spaces if the block stars and ends with spaces.
+  S = FormattedString();

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 198463.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.

- Update a comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58547

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/FormattedString.cpp
  clang-tools-extra/clangd/FormattedString.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/FormattedStringTests.cpp

Index: clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
@@ -0,0 +1,156 @@
+//===-- FormattedStringTests.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "FormattedString.h"
+#include "clang/Basic/LLVM.h"
+#include "llvm/ADT/StringRef.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TEST(FormattedString, Basic) {
+  FormattedString S;
+  EXPECT_EQ(S.renderAsPlainText(), "");
+  EXPECT_EQ(S.renderAsMarkdown(), "");
+
+  S.appendText("foobar");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar");
+  EXPECT_EQ(S.renderAsMarkdown(), "foobar");
+
+  S = FormattedString();
+  S.appendInlineCode("foobar");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar");
+  EXPECT_EQ(S.renderAsMarkdown(), "`foobar`");
+
+  S = FormattedString();
+  S.appendCodeBlock("foobar");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar");
+  EXPECT_EQ(S.renderAsMarkdown(), "```cpp\n"
+  "foobar\n"
+  "```\n");
+}
+
+TEST(FormattedString, CodeBlocks) {
+  FormattedString S;
+  S.appendCodeBlock("foobar");
+  S.appendCodeBlock("bazqux", "javascript");
+
+  EXPECT_EQ(S.renderAsPlainText(), "foobar\n\n\nbazqux");
+  std::string ExpectedMarkdown = R"md(```cpp
+foobar
+```
+```javascript
+bazqux
+```
+)md";
+  EXPECT_EQ(S.renderAsMarkdown(), ExpectedMarkdown);
+
+  S = FormattedString();
+  S.appendInlineCode("foobar");
+  S.appendInlineCode("bazqux");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar bazqux");
+  EXPECT_EQ(S.renderAsMarkdown(), "`foobar` `bazqux`");
+
+  S = FormattedString();
+  S.appendText("foo");
+  S.appendInlineCode("bar");
+  S.appendText("baz");
+
+  EXPECT_EQ(S.renderAsPlainText(), "foo bar baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "foo`bar`baz");
+}
+
+TEST(FormattedString, Escaping) {
+  // Check some ASCII punctuation
+  FormattedString S;
+  S.appendText("*!`");
+  EXPECT_EQ(S.renderAsMarkdown(), "\\*\\!\\`");
+
+  // Check all ASCII punctuation.
+  S = FormattedString();
+  std::string Punctuation = R"txt(!"#$%&'()*+,-./:;<=>?@[\]^_`{|}~)txt";
+  // Same text, with each character escaped.
+  std::string EscapedPunctuation;
+  EscapedPunctuation.reserve(2 * Punctuation.size());
+  for (char C : Punctuation)
+EscapedPunctuation += std::string("\\") + C;
+  S.appendText(Punctuation);
+  EXPECT_EQ(S.renderAsMarkdown(), EscapedPunctuation);
+
+  // In code blocks we don't need to escape ASCII punctuation.
+  S = FormattedString();
+  S.appendInlineCode("* foo !+ bar * baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "`* foo !+ bar * baz`");
+  S = FormattedString();
+  S.appendCodeBlock("#define FOO\n* foo !+ bar * baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "```cpp\n"
+  "#define FOO\n* foo !+ bar * baz\n"
+  "```\n");
+
+  // But we have to escape the backticks.
+  S = FormattedString();
+  S.appendInlineCode("foo`bar`baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "`foo``bar``baz`");
+
+  S = FormattedString();
+  S.appendCodeBlock("foo`bar`baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "```cpp\n"
+  "foo`bar`baz\n"
+  "```\n");
+
+  // Inline code blocks starting or ending with backticks should add spaces.
+  S = FormattedString();
+  S.appendInlineCode("`foo");
+  EXPECT_EQ(S.renderAsMarkdown(), "` ``foo `");
+  S = FormattedString();
+  S.appendInlineCode("foo`");
+  EXPECT_EQ(S.renderAsMarkdown(), "` foo`` `");
+  S = FormattedString();
+  S.appendInlineCode("`foo`");
+  EXPECT_EQ(S.renderAsMarkdown(), "` ``foo`` `");
+
+  // Should also add extra spaces if the block stars and ends with spaces.
+  S = FormattedString();
+  S.appendInlineCode(" foo ");
+  EXPECT_EQ(S.renderAsMarkdown(), "`  foo  `");
+  S = FormattedString();
+  S.appendInlineCode("foo ");
+  EXPECT_EQ(S.renderAsMarkdown(), "`foo `");
+  S = FormattedString();
+  S.appendInlineCode(" foo");
+  EXPECT_EQ(S.renderAsMarkdown(), "` foo`");
+

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/FormattedString.h:34
+  /// markdown.
+  /// EXPECTS: Code does not contain newlines.
+  void appendInlineCode(std::string Code);

newlines are allowed now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58547



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

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/FormattedString.cpp:71
+  // start and end the code block with more.
+  unsigned MaxBackticks = 0;
+  for (llvm::StringRef Left = Input;;) {

sammccall wrote:
> ```
> backticks = "```"
> while (Input.contains(backticks))
>   backticks.push_back("`")
> ```
> 
> I don't think we care about DOS here?
I'd like to keep this linear anyway.
Simplified the algorithm a bit, let me know if it still looks too complicated.



Comment at: clang-tools-extra/clangd/FormattedString.cpp:107
+void FormattedString::appendInlineCode(std::string Code) {
+  assert(!llvm::StringRef(Code).contains("\n"));
+

sammccall wrote:
> I'm not sure we're realistically going to be able to enforce this very well, 
> we're going to use this for types. Maybe translate into ' ' instead? (or at 
> least in production mode)
Actually, the [[ https://spec.commonmark.org/0.29/#code-spans | CommonMark 
specification ]] allows newline in the inline code blocks.
The renderers should replace newlines with spaces.
Removed the assertion.



Comment at: clang-tools-extra/clangd/FormattedString.cpp:140
+
+static bool IsWhitespace(char C) {
+  return llvm::StringLiteral(" \t\n").contains(C);

sammccall wrote:
> (we could also use the version in CharInfo)
Thanks! I knew there should be a helper like this somewhere in LLVM...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58547



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

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 198300.
ilya-biryukov added a comment.

- Remove hover-related bits, they will go into a separate revision


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58547

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/FormattedString.cpp
  clang-tools-extra/clangd/FormattedString.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/FormattedStringTests.cpp

Index: clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
@@ -0,0 +1,156 @@
+//===-- FormattedStringTests.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "FormattedString.h"
+#include "clang/Basic/LLVM.h"
+#include "llvm/ADT/StringRef.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TEST(FormattedString, Basic) {
+  FormattedString S;
+  EXPECT_EQ(S.renderAsPlainText(), "");
+  EXPECT_EQ(S.renderAsMarkdown(), "");
+
+  S.appendText("foobar");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar");
+  EXPECT_EQ(S.renderAsMarkdown(), "foobar");
+
+  S = FormattedString();
+  S.appendInlineCode("foobar");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar");
+  EXPECT_EQ(S.renderAsMarkdown(), "`foobar`");
+
+  S = FormattedString();
+  S.appendCodeBlock("foobar");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar");
+  EXPECT_EQ(S.renderAsMarkdown(), "```cpp\n"
+  "foobar\n"
+  "```\n");
+}
+
+TEST(FormattedString, CodeBlocks) {
+  FormattedString S;
+  S.appendCodeBlock("foobar");
+  S.appendCodeBlock("bazqux", "javascript");
+
+  EXPECT_EQ(S.renderAsPlainText(), "foobar\n\n\nbazqux");
+  std::string ExpectedMarkdown = R"md(```cpp
+foobar
+```
+```javascript
+bazqux
+```
+)md";
+  EXPECT_EQ(S.renderAsMarkdown(), ExpectedMarkdown);
+
+  S = FormattedString();
+  S.appendInlineCode("foobar");
+  S.appendInlineCode("bazqux");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar bazqux");
+  EXPECT_EQ(S.renderAsMarkdown(), "`foobar` `bazqux`");
+
+  S = FormattedString();
+  S.appendText("foo");
+  S.appendInlineCode("bar");
+  S.appendText("baz");
+
+  EXPECT_EQ(S.renderAsPlainText(), "foo bar baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "foo`bar`baz");
+}
+
+TEST(FormattedString, Escaping) {
+  // Check some ASCII punctuation
+  FormattedString S;
+  S.appendText("*!`");
+  EXPECT_EQ(S.renderAsMarkdown(), "\\*\\!\\`");
+
+  // Check all ASCII punctuation.
+  S = FormattedString();
+  std::string Punctuation = R"txt(!"#$%&'()*+,-./:;<=>?@[\]^_`{|}~)txt";
+  // Same text, with each character escaped.
+  std::string EscapedPunctuation;
+  EscapedPunctuation.reserve(2 * Punctuation.size());
+  for (char C : Punctuation)
+EscapedPunctuation += std::string("\\") + C;
+  S.appendText(Punctuation);
+  EXPECT_EQ(S.renderAsMarkdown(), EscapedPunctuation);
+
+  // In code blocks we don't need to escape ASCII punctuation.
+  S = FormattedString();
+  S.appendInlineCode("* foo !+ bar * baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "`* foo !+ bar * baz`");
+  S = FormattedString();
+  S.appendCodeBlock("#define FOO\n* foo !+ bar * baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "```cpp\n"
+  "#define FOO\n* foo !+ bar * baz\n"
+  "```\n");
+
+  // But we have to escape the backticks.
+  S = FormattedString();
+  S.appendInlineCode("foo`bar`baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "`foo``bar``baz`");
+
+  S = FormattedString();
+  S.appendCodeBlock("foo`bar`baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "```cpp\n"
+  "foo`bar`baz\n"
+  "```\n");
+
+  // Inline code blocks starting or ending with backticks should add spaces.
+  S = FormattedString();
+  S.appendInlineCode("`foo");
+  EXPECT_EQ(S.renderAsMarkdown(), "` ``foo `");
+  S = FormattedString();
+  S.appendInlineCode("foo`");
+  EXPECT_EQ(S.renderAsMarkdown(), "` foo`` `");
+  S = FormattedString();
+  S.appendInlineCode("`foo`");
+  EXPECT_EQ(S.renderAsMarkdown(), "` ``foo`` `");
+
+  // Should also add extra spaces if the block stars and ends with spaces.
+  S = FormattedString();
+  S.appendInlineCode(" foo ");
+  EXPECT_EQ(S.renderAsMarkdown(), "`  foo  `");
+  S = FormattedString();
+  S.appendInlineCode("foo ");
+  EXPECT_EQ(S.renderAsMarkdown(), "`foo `");
+  S = FormattedString();
+  S.appendInlineCode(" foo");
+  EXPECT_EQ(S.renderAsMarkdown(), "` foo`");
+

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 198298.
ilya-biryukov marked 7 inline comments as done.
ilya-biryukov added a comment.

- Use isWhitespace from CharInfo.
- Add a comment for merging text blocks together.
- Remove assertion that inline code block does not contain newlines.
- Simplify how the max number of backticks is computed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58547

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FormattedString.cpp
  clang-tools-extra/clangd/FormattedString.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
  clang-tools-extra/clangd/unittests/XRefsTests.cpp

Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -1185,7 +1185,9 @@
 auto AST = TU.build();
 if (auto H = getHover(AST, T.point())) {
   EXPECT_NE("", Test.ExpectedHover) << Test.Input;
-  EXPECT_EQ(H->contents.value, Test.ExpectedHover.str()) << Test.Input;
+  // FIXME: add renderForTests() and use it here.
+  EXPECT_EQ(H->Content.renderAsPlainText(), Test.ExpectedHover.str())
+  << Test.Input;
 } else
   EXPECT_EQ("", Test.ExpectedHover.str()) << Test.Input;
   }
Index: clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
@@ -0,0 +1,156 @@
+//===-- FormattedStringTests.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "FormattedString.h"
+#include "clang/Basic/LLVM.h"
+#include "llvm/ADT/StringRef.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TEST(FormattedString, Basic) {
+  FormattedString S;
+  EXPECT_EQ(S.renderAsPlainText(), "");
+  EXPECT_EQ(S.renderAsMarkdown(), "");
+
+  S.appendText("foobar");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar");
+  EXPECT_EQ(S.renderAsMarkdown(), "foobar");
+
+  S = FormattedString();
+  S.appendInlineCode("foobar");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar");
+  EXPECT_EQ(S.renderAsMarkdown(), "`foobar`");
+
+  S = FormattedString();
+  S.appendCodeBlock("foobar");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar");
+  EXPECT_EQ(S.renderAsMarkdown(), "```cpp\n"
+  "foobar\n"
+  "```\n");
+}
+
+TEST(FormattedString, CodeBlocks) {
+  FormattedString S;
+  S.appendCodeBlock("foobar");
+  S.appendCodeBlock("bazqux", "javascript");
+
+  EXPECT_EQ(S.renderAsPlainText(), "foobar\n\n\nbazqux");
+  std::string ExpectedMarkdown = R"md(```cpp
+foobar
+```
+```javascript
+bazqux
+```
+)md";
+  EXPECT_EQ(S.renderAsMarkdown(), ExpectedMarkdown);
+
+  S = FormattedString();
+  S.appendInlineCode("foobar");
+  S.appendInlineCode("bazqux");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar bazqux");
+  EXPECT_EQ(S.renderAsMarkdown(), "`foobar` `bazqux`");
+
+  S = FormattedString();
+  S.appendText("foo");
+  S.appendInlineCode("bar");
+  S.appendText("baz");
+
+  EXPECT_EQ(S.renderAsPlainText(), "foo bar baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "foo`bar`baz");
+}
+
+TEST(FormattedString, Escaping) {
+  // Check some ASCII punctuation
+  FormattedString S;
+  S.appendText("*!`");
+  EXPECT_EQ(S.renderAsMarkdown(), "\\*\\!\\`");
+
+  // Check all ASCII punctuation.
+  S = FormattedString();
+  std::string Punctuation = R"txt(!"#$%&'()*+,-./:;<=>?@[\]^_`{|}~)txt";
+  // Same text, with each character escaped.
+  std::string EscapedPunctuation;
+  EscapedPunctuation.reserve(2 * Punctuation.size());
+  for (char C : Punctuation)
+EscapedPunctuation += std::string("\\") + C;
+  S.appendText(Punctuation);
+  EXPECT_EQ(S.renderAsMarkdown(), EscapedPunctuation);
+
+  // In code blocks we don't need to escape ASCII punctuation.
+  S = FormattedString();
+  S.appendInlineCode("* foo !+ bar * baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "`* foo !+ bar * baz`");
+  S = FormattedString();
+  S.appendCodeBlock("#define FOO\n* foo !+ bar * baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "```cpp\n"
+  

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

As discussed, it probably makes sense to split into two patches, adding this 
abstraction and using it.
(All together is also fine but should have the tests for the new behavior, 
which probably means renderForTests() too)

Generally looks good. I think we'll run into the limitations as soon as we want 
to add lists, but that's OK.




Comment at: clang-tools-extra/clangd/FormattedString.cpp:71
+  // start and end the code block with more.
+  unsigned MaxBackticks = 0;
+  for (llvm::StringRef Left = Input;;) {

```
backticks = "```"
while (Input.contains(backticks))
  backticks.push_back("`")
```

I don't think we care about DOS here?



Comment at: clang-tools-extra/clangd/FormattedString.cpp:90
+void FormattedString::appendText(std::string Text) {
+  if (Chunks.empty() || Chunks.back().Kind != ChunkKind::PlainText) {
+Chunk C;

comment for this merge?



Comment at: clang-tools-extra/clangd/FormattedString.cpp:107
+void FormattedString::appendInlineCode(std::string Code) {
+  assert(!llvm::StringRef(Code).contains("\n"));
+

I'm not sure we're realistically going to be able to enforce this very well, 
we're going to use this for types. Maybe translate into ' ' instead? (or at 
least in production mode)



Comment at: clang-tools-extra/clangd/FormattedString.cpp:140
+
+static bool IsWhitespace(char C) {
+  return llvm::StringLiteral(" \t\n").contains(C);

(we could also use the version in CharInfo)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58547



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

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 197994.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.

- Remove dead test code. NFC


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58547

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FormattedString.cpp
  clang-tools-extra/clangd/FormattedString.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
  clang-tools-extra/clangd/unittests/XRefsTests.cpp

Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -1172,7 +1172,9 @@
 auto AST = TU.build();
 if (auto H = getHover(AST, T.point())) {
   EXPECT_NE("", Test.ExpectedHover) << Test.Input;
-  EXPECT_EQ(H->contents.value, Test.ExpectedHover.str()) << Test.Input;
+  // FIXME: add renderForTests() and use it here.
+  EXPECT_EQ(H->Content.renderAsPlainText(), Test.ExpectedHover.str())
+  << Test.Input;
 } else
   EXPECT_EQ("", Test.ExpectedHover.str()) << Test.Input;
   }
Index: clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
@@ -0,0 +1,156 @@
+//===-- FormattedStringTests.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "FormattedString.h"
+#include "clang/Basic/LLVM.h"
+#include "llvm/ADT/StringRef.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TEST(FormattedString, Basic) {
+  FormattedString S;
+  EXPECT_EQ(S.renderAsPlainText(), "");
+  EXPECT_EQ(S.renderAsMarkdown(), "");
+
+  S.appendText("foobar");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar");
+  EXPECT_EQ(S.renderAsMarkdown(), "foobar");
+
+  S = FormattedString();
+  S.appendInlineCode("foobar");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar");
+  EXPECT_EQ(S.renderAsMarkdown(), "`foobar`");
+
+  S = FormattedString();
+  S.appendCodeBlock("foobar");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar");
+  EXPECT_EQ(S.renderAsMarkdown(), "```cpp\n"
+  "foobar\n"
+  "```\n");
+}
+
+TEST(FormattedString, CodeBlocks) {
+  FormattedString S;
+  S.appendCodeBlock("foobar");
+  S.appendCodeBlock("bazqux", "javascript");
+
+  EXPECT_EQ(S.renderAsPlainText(), "foobar\n\n\nbazqux");
+  std::string ExpectedMarkdown = R"md(```cpp
+foobar
+```
+```javascript
+bazqux
+```
+)md";
+  EXPECT_EQ(S.renderAsMarkdown(), ExpectedMarkdown);
+
+  S = FormattedString();
+  S.appendInlineCode("foobar");
+  S.appendInlineCode("bazqux");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar bazqux");
+  EXPECT_EQ(S.renderAsMarkdown(), "`foobar` `bazqux`");
+
+  S = FormattedString();
+  S.appendText("foo");
+  S.appendInlineCode("bar");
+  S.appendText("baz");
+
+  EXPECT_EQ(S.renderAsPlainText(), "foo bar baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "foo`bar`baz");
+}
+
+TEST(FormattedString, Escaping) {
+  // Check some ASCII punctuation
+  FormattedString S;
+  S.appendText("*!`");
+  EXPECT_EQ(S.renderAsMarkdown(), "\\*\\!\\`");
+
+  // Check all ASCII punctuation.
+  S = FormattedString();
+  std::string Punctuation = R"txt(!"#$%&'()*+,-./:;<=>?@[\]^_`{|}~)txt";
+  // Same text, with each character escaped.
+  std::string EscapedPunctuation;
+  EscapedPunctuation.reserve(2 * Punctuation.size());
+  for (char C : Punctuation)
+EscapedPunctuation += std::string("\\") + C;
+  S.appendText(Punctuation);
+  EXPECT_EQ(S.renderAsMarkdown(), EscapedPunctuation);
+
+  // In code blocks we don't need to escape ASCII punctuation.
+  S = FormattedString();
+  S.appendInlineCode("* foo !+ bar * baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "`* foo !+ bar * baz`");
+  S = FormattedString();
+  S.appendCodeBlock("#define FOO\n* foo !+ bar * baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "```cpp\n"
+  "#define FOO\n* foo !+ bar * baz\n"
+  "```\n");
+
+  // But we have to escape the backticks.
+  S = FormattedString();
+  

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

This is ready for another round of review. As mentioned in the comments, I 
think some of the comments are better addressed by a separate change, because 
they would touch lots of lines in the test.
Let me know if I missed anything else.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58547



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

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.h:186
   void findHover(PathRef File, Position Pos,
- Callback> CB);
+ Callback> CB);
 

ilya-biryukov wrote:
> kadircet wrote:
> > sammccall wrote:
> > > Not sure about switching from `Hover` to `FormattedString` as return type 
> > > here: LSP hover struct contains a range field (what are the bounds of the 
> > > hovered thing?) that we may want to populate that doesn't fit in 
> > > FormattedString.
> > > I'd suggest the function in XRefs.h should return `FormattedString` (and 
> > > later maybe `pair`), and the `ClangdServer` 
> > > version should continue to provide the rendered `Hover`.
> > > 
> > > (We'll eventually want ClangdServer to return some HoverInfo struct with 
> > > structured information, as we want embedding clients to be able to render 
> > > it other ways, and maybe use it to provide extension methods like YCM's 
> > > `GetType`. But no need to touch that in this patch, we'll end up 
> > > producing HoverInfo -> FormattedString -> LSP Hover, I guess)
> > I agree with Sam on the layering. I was also working in a patch that was 
> > changing `ClangdServer`s output from `Hover` to `HoverInfo`(a structured 
> > version of `Hover`).
> `ClangdServer` does not know whether to render the `FormattedString` into 
> markdown or plaintext and I believe it should stay that way.
> Happy to return `HoverInfo`, though. I'll introduce one with 
> `FormattedString` and `Range`
Returning `HoverInfo` now, with a `FormattedString` and a range.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58547



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

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/FormattedString.h:27
+  /// Append plain text to the end of the string.
+  void appendText(std::string Text);
+  /// Append a block of C++ code. This translates to a ``` block in markdown.

sammccall wrote:
> I guess this will get an optional parameter to control the style (bold, 
> italic etc)?
Either that or a separate function to add bold/italic, etc.
I think we should keep it simple in the first revision.

This really tries to provide a vocabulary type to carry a string with formatted 
blocks around, not support full markdown from the start.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58547



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

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 197991.
ilya-biryukov added a comment.

- Remove a FIXME that was fixed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58547

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FormattedString.cpp
  clang-tools-extra/clangd/FormattedString.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
  clang-tools-extra/clangd/unittests/XRefsTests.cpp

Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -1172,7 +1172,9 @@
 auto AST = TU.build();
 if (auto H = getHover(AST, T.point())) {
   EXPECT_NE("", Test.ExpectedHover) << Test.Input;
-  EXPECT_EQ(H->contents.value, Test.ExpectedHover.str()) << Test.Input;
+  // FIXME: add renderForTests() and use it here.
+  EXPECT_EQ(H->Content.renderAsPlainText(), Test.ExpectedHover.str())
+  << Test.Input;
 } else
   EXPECT_EQ("", Test.ExpectedHover.str()) << Test.Input;
   }
Index: clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
@@ -0,0 +1,158 @@
+//===-- FormattedStringTests.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "FormattedString.h"
+#include "clang/Basic/LLVM.h"
+#include "llvm/ADT/StringRef.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TEST(FormattedString, Basic) {
+  FormattedString S;
+  EXPECT_EQ(S.renderAsPlainText(), "");
+  EXPECT_EQ(S.renderAsMarkdown(), "");
+
+  S.appendText("foobar");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar");
+  EXPECT_EQ(S.renderAsMarkdown(), "foobar");
+
+  S = FormattedString();
+  S.appendInlineCode("foobar");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar");
+  EXPECT_EQ(S.renderAsMarkdown(), "`foobar`");
+
+  S = FormattedString();
+  S.appendCodeBlock("foobar");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar");
+  EXPECT_EQ(S.renderAsMarkdown(), "```cpp\n"
+  "foobar\n"
+  "```\n");
+
+  S = FormattedString();
+}
+
+TEST(FormattedString, CodeBlocks) {
+  FormattedString S;
+  S.appendCodeBlock("foobar");
+  S.appendCodeBlock("bazqux", "javascript");
+
+  EXPECT_EQ(S.renderAsPlainText(), "foobar\n\n\nbazqux");
+  std::string ExpectedMarkdown = R"md(```cpp
+foobar
+```
+```javascript
+bazqux
+```
+)md";
+  EXPECT_EQ(S.renderAsMarkdown(), ExpectedMarkdown);
+
+  S = FormattedString();
+  S.appendInlineCode("foobar");
+  S.appendInlineCode("bazqux");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar bazqux");
+  EXPECT_EQ(S.renderAsMarkdown(), "`foobar` `bazqux`");
+
+  S = FormattedString();
+  S.appendText("foo");
+  S.appendInlineCode("bar");
+  S.appendText("baz");
+
+  EXPECT_EQ(S.renderAsPlainText(), "foo bar baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "foo`bar`baz");
+}
+
+TEST(FormattedString, Escaping) {
+  // Check some ASCII punctuation
+  FormattedString S;
+  S.appendText("*!`");
+  EXPECT_EQ(S.renderAsMarkdown(), "\\*\\!\\`");
+
+  // Check all ASCII punctuation.
+  S = FormattedString();
+  std::string Punctuation = R"txt(!"#$%&'()*+,-./:;<=>?@[\]^_`{|}~)txt";
+  // Same text, with each character escaped.
+  std::string EscapedPunctuation;
+  EscapedPunctuation.reserve(2 * Punctuation.size());
+  for (char C : Punctuation)
+EscapedPunctuation += std::string("\\") + C;
+  S.appendText(Punctuation);
+  EXPECT_EQ(S.renderAsMarkdown(), EscapedPunctuation);
+
+  // In code blocks we don't need to escape ASCII punctuation.
+  S = FormattedString();
+  S.appendInlineCode("* foo !+ bar * baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "`* foo !+ bar * baz`");
+  S = FormattedString();
+  S.appendCodeBlock("#define FOO\n* foo !+ bar * baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "```cpp\n"
+  "#define FOO\n* foo !+ bar * baz\n"
+  "```\n");
+
+  // But we have to escape the backticks.
+  S = FormattedString();
+  

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 197989.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.

- Return 'HoverInfo' instead of FormattedString
- Reformat


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58547

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FormattedString.cpp
  clang-tools-extra/clangd/FormattedString.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
  clang-tools-extra/clangd/unittests/XRefsTests.cpp

Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -1172,7 +1172,9 @@
 auto AST = TU.build();
 if (auto H = getHover(AST, T.point())) {
   EXPECT_NE("", Test.ExpectedHover) << Test.Input;
-  EXPECT_EQ(H->contents.value, Test.ExpectedHover.str()) << Test.Input;
+  // FIXME: add renderForTests() and use it here.
+  EXPECT_EQ(H->Content.renderAsPlainText(), Test.ExpectedHover.str())
+  << Test.Input;
 } else
   EXPECT_EQ("", Test.ExpectedHover.str()) << Test.Input;
   }
Index: clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
@@ -0,0 +1,158 @@
+//===-- FormattedStringTests.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "FormattedString.h"
+#include "clang/Basic/LLVM.h"
+#include "llvm/ADT/StringRef.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TEST(FormattedString, Basic) {
+  FormattedString S;
+  EXPECT_EQ(S.renderAsPlainText(), "");
+  EXPECT_EQ(S.renderAsMarkdown(), "");
+
+  S.appendText("foobar");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar");
+  EXPECT_EQ(S.renderAsMarkdown(), "foobar");
+
+  S = FormattedString();
+  S.appendInlineCode("foobar");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar");
+  EXPECT_EQ(S.renderAsMarkdown(), "`foobar`");
+
+  S = FormattedString();
+  S.appendCodeBlock("foobar");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar");
+  EXPECT_EQ(S.renderAsMarkdown(), "```cpp\n"
+  "foobar\n"
+  "```\n");
+
+  S = FormattedString();
+}
+
+TEST(FormattedString, CodeBlocks) {
+  FormattedString S;
+  S.appendCodeBlock("foobar");
+  S.appendCodeBlock("bazqux", "javascript");
+
+  EXPECT_EQ(S.renderAsPlainText(), "foobar\n\n\nbazqux");
+  std::string ExpectedMarkdown = R"md(```cpp
+foobar
+```
+```javascript
+bazqux
+```
+)md";
+  EXPECT_EQ(S.renderAsMarkdown(), ExpectedMarkdown);
+
+  S = FormattedString();
+  S.appendInlineCode("foobar");
+  S.appendInlineCode("bazqux");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar bazqux");
+  EXPECT_EQ(S.renderAsMarkdown(), "`foobar` `bazqux`");
+
+  S = FormattedString();
+  S.appendText("foo");
+  S.appendInlineCode("bar");
+  S.appendText("baz");
+
+  EXPECT_EQ(S.renderAsPlainText(), "foo bar baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "foo`bar`baz");
+}
+
+TEST(FormattedString, Escaping) {
+  // Check some ASCII punctuation
+  FormattedString S;
+  S.appendText("*!`");
+  EXPECT_EQ(S.renderAsMarkdown(), "\\*\\!\\`");
+
+  // Check all ASCII punctuation.
+  S = FormattedString();
+  std::string Punctuation = R"txt(!"#$%&'()*+,-./:;<=>?@[\]^_`{|}~)txt";
+  // Same text, with each character escaped.
+  std::string EscapedPunctuation;
+  EscapedPunctuation.reserve(2 * Punctuation.size());
+  for (char C : Punctuation)
+EscapedPunctuation += std::string("\\") + C;
+  S.appendText(Punctuation);
+  EXPECT_EQ(S.renderAsMarkdown(), EscapedPunctuation);
+
+  // In code blocks we don't need to escape ASCII punctuation.
+  S = FormattedString();
+  S.appendInlineCode("* foo !+ bar * baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "`* foo !+ bar * baz`");
+  S = FormattedString();
+  S.appendCodeBlock("#define FOO\n* foo !+ bar * baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "```cpp\n"
+  "#define FOO\n* foo !+ bar * baz\n"
+  "```\n");
+
+  // But we have 

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.h:186
   void findHover(PathRef File, Position Pos,
- Callback> CB);
+ Callback> CB);
 

kadircet wrote:
> sammccall wrote:
> > Not sure about switching from `Hover` to `FormattedString` as return type 
> > here: LSP hover struct contains a range field (what are the bounds of the 
> > hovered thing?) that we may want to populate that doesn't fit in 
> > FormattedString.
> > I'd suggest the function in XRefs.h should return `FormattedString` (and 
> > later maybe `pair`), and the `ClangdServer` version 
> > should continue to provide the rendered `Hover`.
> > 
> > (We'll eventually want ClangdServer to return some HoverInfo struct with 
> > structured information, as we want embedding clients to be able to render 
> > it other ways, and maybe use it to provide extension methods like YCM's 
> > `GetType`. But no need to touch that in this patch, we'll end up producing 
> > HoverInfo -> FormattedString -> LSP Hover, I guess)
> I agree with Sam on the layering. I was also working in a patch that was 
> changing `ClangdServer`s output from `Hover` to `HoverInfo`(a structured 
> version of `Hover`).
`ClangdServer` does not know whether to render the `FormattedString` into 
markdown or plaintext and I believe it should stay that way.
Happy to return `HoverInfo`, though. I'll introduce one with `FormattedString` 
and `Range`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58547



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

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/FormattedString.cpp:41
+
+void FormattedString::appendCodeBlock(std::string Code) {
+  Chunk C;

sammccall wrote:
> you may want to take the language name, and default it to either cpp or 
> nothing.
> Including it in the triple-backtick block can trigger syntax highlighting, 
> and editors are probably somewhat likely to actually implement this.
Done. Defaulting to 'cpp'.
VSCode actually does syntax highlighting there and it looks nice.



Comment at: clang-tools-extra/clangd/FormattedString.cpp:63
+case ChunkKind::InlineCodeBlock:
+  R += " `";
+  R += escapeBackticks(C.Contents);

sammccall wrote:
> why do we add surrounding spaces if we don't do the same for text chunks?
a leftover from my first prototype.
They don't seem to be necessary, removed them.



Comment at: clang-tools-extra/clangd/FormattedString.h:31
+  /// newlines.
+  void appendCodeBlock(std::string Code);
+  /// Append an inline block of C++ code. This translates to the ` block in

sammccall wrote:
> Having various methods that take strings may struggle if we want a lot of 
> composability (e.g. a bullet list whose bullet whose third word is a 
> hyperlink/bold).
> 
> (I'm not sure whether this is going to be a real problem, just thinking out 
> loud here)
> 
> One alternative extensibility would be to make "containers" methods taking a 
> lambda that renders the body.
> e.g.
> 
> ```
> FormattedString S;
> S << "std::";
> S.bold([&} S << "swap" };
> S.list(FormattedString::Numbered, [&] {
>   S.item([&] { S << "sometimes you get the wrong overload"; });
>   S.item([&] {
> S << "watch out for ";
> S.link("https://en.cppreference.com/w/cpp/language/adl;, [&] { S << 
> "ADL"; });
>   });
> });
> S.codeBlock([&] S << "Here is an example"; });
> ```
> 
> Not sure if this is really better, I just have it on my mind because I ended 
> up using it for the `JSON.h` streaming output. We can probably bolt it on 
> later if necessary.
Agree that the current approach won't generalize to more complicated cases 
without some design work.
The lambda-heavy code is hard to read to my personal taste, but may actually be 
a good option in practice.
I'd also try playing around with some lambda-free alternatives to see if they 
lead to better syntax without compromising extensibility or making the 
implementation overly complex.

I also think we should bolt on this until we hit more use-cases with more 
extensive needs for formatting markdown. 



Comment at: clang-tools-extra/clangd/FormattedString.h:39
+  std::string renderAsPlainText() const;
+
+private:

sammccall wrote:
> I think we want renderForDebugging() or so which would print the chunks 
> explicitly, a la CodeCompletionString.
> 
> This is useful for actual debugging, and for testing methods that return 
> FormattedString (as opposed to the FormattedString->markdown translation)
That would be a useful addition. I've added a FIXME for now and testing 
plaintext in XRefs.
I'd suggest adding debug representation in a follow-up change, but also happy 
to do it now if you feel that's necessary.

That would need updates to all hover tests and that's a big enough to go into a 
separate change, from my perspective.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58547



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

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 197986.
ilya-biryukov marked 7 inline comments as done.
ilya-biryukov added a comment.

- Rebase
- Parse client capabilities, send markdown on hover if client supports it
- Use inline block for the scope
- Add a language for code blocks
- Add a FIXME for hover tests
- Fix escaping, add tests
- Update XRefs tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58547

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FormattedString.cpp
  clang-tools-extra/clangd/FormattedString.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
  clang-tools-extra/clangd/unittests/XRefsTests.cpp

Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -1172,7 +1172,8 @@
 auto AST = TU.build();
 if (auto H = getHover(AST, T.point())) {
   EXPECT_NE("", Test.ExpectedHover) << Test.Input;
-  EXPECT_EQ(H->contents.value, Test.ExpectedHover.str()) << Test.Input;
+  // FIXME: add renderForTests() and use it here.
+  EXPECT_EQ(H->renderAsPlainText(), Test.ExpectedHover.str()) << Test.Input;
 } else
   EXPECT_EQ("", Test.ExpectedHover.str()) << Test.Input;
   }
Index: clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
@@ -0,0 +1,158 @@
+//===-- FormattedStringTests.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "FormattedString.h"
+#include "clang/Basic/LLVM.h"
+#include "llvm/ADT/StringRef.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TEST(FormattedString, Basic) {
+  FormattedString S;
+  EXPECT_EQ(S.renderAsPlainText(), "");
+  EXPECT_EQ(S.renderAsMarkdown(), "");
+
+  S.appendText("foobar");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar");
+  EXPECT_EQ(S.renderAsMarkdown(), "foobar");
+
+  S = FormattedString();
+  S.appendInlineCode("foobar");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar");
+  EXPECT_EQ(S.renderAsMarkdown(), "`foobar`");
+
+  S = FormattedString();
+  S.appendCodeBlock("foobar");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar");
+  EXPECT_EQ(S.renderAsMarkdown(), "```cpp\n"
+  "foobar\n"
+  "```\n");
+
+  S = FormattedString();
+}
+
+TEST(FormattedString, CodeBlocks) {
+  FormattedString S;
+  S.appendCodeBlock("foobar");
+  S.appendCodeBlock("bazqux", "javascript");
+
+  EXPECT_EQ(S.renderAsPlainText(), "foobar\n\n\nbazqux");
+  std::string ExpectedMarkdown = R"md(```cpp
+foobar
+```
+```javascript
+bazqux
+```
+)md";
+  EXPECT_EQ(S.renderAsMarkdown(), ExpectedMarkdown);
+
+  S = FormattedString();
+  S.appendInlineCode("foobar");
+  S.appendInlineCode("bazqux");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar bazqux");
+  EXPECT_EQ(S.renderAsMarkdown(), "`foobar` `bazqux`");
+
+  S = FormattedString();
+  S.appendText("foo");
+  S.appendInlineCode("bar");
+  S.appendText("baz");
+
+  EXPECT_EQ(S.renderAsPlainText(), "foo bar baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "foo`bar`baz");
+}
+
+TEST(FormattedString, Escaping) {
+  // Check some ASCII punctuation
+  FormattedString S;
+  S.appendText("*!`");
+  EXPECT_EQ(S.renderAsMarkdown(), "\\*\\!\\`");
+
+  // Check all ASCII punctuation.
+  S = FormattedString();
+  std::string Punctuation = R"txt(!"#$%&'()*+,-./:;<=>?@[\]^_`{|}~)txt";
+  // Same text, with each character escaped.
+  std::string EscapedPunctuation;
+  EscapedPunctuation.reserve(2 * Punctuation.size());
+  for (char C : Punctuation)
+EscapedPunctuation += std::string("\\") + C;
+  S.appendText(Punctuation);
+  EXPECT_EQ(S.renderAsMarkdown(), EscapedPunctuation);
+
+  // In code blocks we don't need to escape ASCII punctuation.
+  S = FormattedString();
+  S.appendInlineCode("* foo !+ bar * baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "`* foo !+ bar * baz`");
+  S = FormattedString();
+  S.appendCodeBlock("#define FOO\n* foo !+ bar * baz");
+  EXPECT_EQ(S.renderAsMarkdown(), 

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.h:186
   void findHover(PathRef File, Position Pos,
- Callback> CB);
+ Callback> CB);
 

sammccall wrote:
> Not sure about switching from `Hover` to `FormattedString` as return type 
> here: LSP hover struct contains a range field (what are the bounds of the 
> hovered thing?) that we may want to populate that doesn't fit in 
> FormattedString.
> I'd suggest the function in XRefs.h should return `FormattedString` (and 
> later maybe `pair`), and the `ClangdServer` version 
> should continue to provide the rendered `Hover`.
> 
> (We'll eventually want ClangdServer to return some HoverInfo struct with 
> structured information, as we want embedding clients to be able to render it 
> other ways, and maybe use it to provide extension methods like YCM's 
> `GetType`. But no need to touch that in this patch, we'll end up producing 
> HoverInfo -> FormattedString -> LSP Hover, I guess)
I agree with Sam on the layering. I was also working in a patch that was 
changing `ClangdServer`s output from `Hover` to `HoverInfo`(a structured 
version of `Hover`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58547



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

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This looks pretty nice to me! Sorry for the wait.
Adding @kadircet as hover-guru-in-waiting.




Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:814
+return Reply(llvm::None);
+  // FIXME: render as markdown if client supports it.
+  Hover R;

(I'd like to see that in this patch if possible, it shouldn't be much work)



Comment at: clang-tools-extra/clangd/ClangdServer.h:186
   void findHover(PathRef File, Position Pos,
- Callback> CB);
+ Callback> CB);
 

Not sure about switching from `Hover` to `FormattedString` as return type here: 
LSP hover struct contains a range field (what are the bounds of the hovered 
thing?) that we may want to populate that doesn't fit in FormattedString.
I'd suggest the function in XRefs.h should return `FormattedString` (and later 
maybe `pair`), and the `ClangdServer` version should 
continue to provide the rendered `Hover`.

(We'll eventually want ClangdServer to return some HoverInfo struct with 
structured information, as we want embedding clients to be able to render it 
other ways, and maybe use it to provide extension methods like YCM's `GetType`. 
But no need to touch that in this patch, we'll end up producing HoverInfo -> 
FormattedString -> LSP Hover, I guess)



Comment at: clang-tools-extra/clangd/FormattedString.cpp:41
+
+void FormattedString::appendCodeBlock(std::string Code) {
+  Chunk C;

you may want to take the language name, and default it to either cpp or nothing.
Including it in the triple-backtick block can trigger syntax highlighting, and 
editors are probably somewhat likely to actually implement this.



Comment at: clang-tools-extra/clangd/FormattedString.cpp:63
+case ChunkKind::InlineCodeBlock:
+  R += " `";
+  R += escapeBackticks(C.Contents);

why do we add surrounding spaces if we don't do the same for text chunks?



Comment at: clang-tools-extra/clangd/FormattedString.h:1
+//===--- FormattedString.h --*- 
C++-*--===//
+//

This will need tests as you note, which shouldn't be hard.
It's not terribly complicated, but it's the sort of thing that may accumulate 
special cases.



Comment at: clang-tools-extra/clangd/FormattedString.h:27
+  /// Append plain text to the end of the string.
+  void appendText(std::string Text);
+  /// Append a block of C++ code. This translates to a ``` block in markdown.

I guess this will get an optional parameter to control the style (bold, italic 
etc)?



Comment at: clang-tools-extra/clangd/FormattedString.h:31
+  /// newlines.
+  void appendCodeBlock(std::string Code);
+  /// Append an inline block of C++ code. This translates to the ` block in

Having various methods that take strings may struggle if we want a lot of 
composability (e.g. a bullet list whose bullet whose third word is a 
hyperlink/bold).

(I'm not sure whether this is going to be a real problem, just thinking out 
loud here)

One alternative extensibility would be to make "containers" methods taking a 
lambda that renders the body.
e.g.

```
FormattedString S;
S << "std::";
S.bold([&} S << "swap" };
S.list(FormattedString::Numbered, [&] {
  S.item([&] { S << "sometimes you get the wrong overload"; });
  S.item([&] {
S << "watch out for ";
S.link("https://en.cppreference.com/w/cpp/language/adl;, [&] { S << "ADL"; 
});
  });
});
S.codeBlock([&] S << "Here is an example"; });
```

Not sure if this is really better, I just have it on my mind because I ended up 
using it for the `JSON.h` streaming output. We can probably bolt it on later if 
necessary.



Comment at: clang-tools-extra/clangd/FormattedString.h:39
+  std::string renderAsPlainText() const;
+
+private:

I think we want renderForDebugging() or so which would print the chunks 
explicitly, a la CodeCompletionString.

This is useful for actual debugging, and for testing methods that return 
FormattedString (as opposed to the FormattedString->markdown translation)



Comment at: clang-tools-extra/clangd/XRefs.cpp:528
+R.appendText("Declared in ");
+R.appendText(*NamedScope);
+R.appendText("\n");

should this be an inline code block?



Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1157
   EXPECT_NE("", Test.ExpectedHover) << Test.Input;
-  EXPECT_EQ(H->contents.value, Test.ExpectedHover.str()) << Test.Input;
+  EXPECT_EQ(H->renderAsPlainText(), Test.ExpectedHover.str()) << 
Test.Input;
 } else

When this lands for real, I think we should assert on the renderForDebugging() 
output or similar.


Repository:
  rG LLVM Github Monorepo

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

This is a follow-up for a discussion from D55250 
.
Still missing test, wanted to get some input on the API first.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58547



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

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov created this revision.
ilya-biryukov added reviewers: malaperle, sammccall, ioeric.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, mgorny.
Herald added a project: clang.

That can render to markdown or plain text. Used for findHover requests.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D58547

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FormattedString.cpp
  clang-tools-extra/clangd/FormattedString.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -1154,7 +1154,7 @@
 auto AST = TU.build();
 if (auto H = getHover(AST, T.point())) {
   EXPECT_NE("", Test.ExpectedHover) << Test.Input;
-  EXPECT_EQ(H->contents.value, Test.ExpectedHover.str()) << Test.Input;
+  EXPECT_EQ(H->renderAsPlainText(), Test.ExpectedHover.str()) << Test.Input;
 } else
   EXPECT_EQ("", Test.ExpectedHover.str()) << Test.Input;
   }
Index: clang-tools-extra/clangd/XRefs.h
===
--- clang-tools-extra/clangd/XRefs.h
+++ clang-tools-extra/clangd/XRefs.h
@@ -14,6 +14,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_XREFS_H
 
 #include "ClangdUnit.h"
+#include "FormattedString.h"
 #include "Protocol.h"
 #include "index/Index.h"
 #include "llvm/ADT/Optional.h"
@@ -47,7 +48,7 @@
   Position Pos);
 
 /// Get the hover information when hovering at \p Pos.
-llvm::Optional getHover(ParsedAST , Position Pos);
+llvm::Optional getHover(ParsedAST , Position Pos);
 
 /// Returns reference locations of the symbol at a specified \p Pos.
 /// \p Limit limits the number of results returned (0 means no limit).
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -7,6 +7,7 @@
 //===--===//
 #include "XRefs.h"
 #include "AST.h"
+#include "FormattedString.h"
 #include "Logger.h"
 #include "SourceCode.h"
 #include "URI.h"
@@ -515,17 +516,17 @@
 }
 
 /// Generate a \p Hover object given the declaration \p D.
-static Hover getHoverContents(const Decl *D) {
-  Hover H;
+static FormattedString getHoverContents(const Decl *D) {
+  FormattedString R;
   llvm::Optional NamedScope = getScopeName(D);
 
   // Generate the "Declared in" section.
   if (NamedScope) {
 assert(!NamedScope->empty());
 
-H.contents.value += "Declared in ";
-H.contents.value += *NamedScope;
-H.contents.value += "\n\n";
+R.appendText("Declared in ");
+R.appendText(*NamedScope);
+R.appendText("\n");
   }
 
   // We want to include the template in the Hover.
@@ -537,35 +538,30 @@
 
   PrintingPolicy Policy =
   printingPolicyForDecls(D->getASTContext().getPrintingPolicy());
-
   D->print(OS, Policy);
 
-  OS.flush();
-
-  H.contents.value += DeclText;
-  return H;
+  R.appendCodeBlock(OS.str());
+  return R;
 }
 
 /// Generate a \p Hover object given the type \p T.
-static Hover getHoverContents(QualType T, ASTContext ) {
-  Hover H;
-  std::string TypeText;
-  llvm::raw_string_ostream OS(TypeText);
+static FormattedString getHoverContents(QualType T, ASTContext ) {
+  FormattedString R;
+
+  std::string Code;
+  llvm::raw_string_ostream OS(Code);
   PrintingPolicy Policy = printingPolicyForDecls(ASTCtx.getPrintingPolicy());
   T.print(OS, Policy);
-  OS.flush();
-  H.contents.value += TypeText;
-  return H;
+
+  R.appendCodeBlock(OS.str());
+  return R;
 }
 
 /// Generate a \p Hover object given the macro \p MacroInf.
-static Hover getHoverContents(llvm::StringRef MacroName) {
-  Hover H;
-
-  H.contents.value = "#define ";
-  H.contents.value += MacroName;
-
-  return H;
+static FormattedString getHoverContents(llvm::StringRef MacroName) {
+  FormattedString S;
+  S.appendCodeBlock("#define " + MacroName.str());
+  return S;
 }
 
 namespace {
@@ -680,7 +676,7 @@
   return V.getDeducedType();
 }
 
-llvm::Optional getHover(ParsedAST , Position Pos) {
+llvm::Optional getHover(ParsedAST , Position Pos) {
   const SourceManager  = AST.getASTContext().getSourceManager();
   SourceLocation SourceLocationBeg =
   getBeginningOfIdentifier(AST, Pos, SourceMgr.getMainFileID());
Index: clang-tools-extra/clangd/FormattedString.h
===
--- /dev/null
+++ clang-tools-extra/clangd/FormattedString.h
@@ -0,0 +1,56 @@
+//===--- FormattedString.h