subject:"\[PATCH\] D71248\: \[clangd\] Introduce paragraph, the first part of new rendering structs"

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

2019-12-19 Thread Kadir Cetinkaya via Phabricator via cfe-commits

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



Comment at: clang-tools-extra/clangd/FormattedString.cpp:110
+// Separate from next block.
+*WritePtr++ = ' ';
+  }

aganea wrote:
> There's an issue at this line, the iterator might go past the end of the 
> string (if running the `Document.Separators` test). This fails when running a 
> Debug build with VS 2019. The code fails with this assert (in `xstring`):
> ```
> _STL_VERIFY(_Unfancy(_Ptr) < _Mycont->_Myptr() + _Mycont->_Mysize, "cannot 
> increment string iterator past end");
> ```
> I changed the code such as: {F11108172}
> (sorry, Phabricator throws an exception when uploading this patch as a 
> differential)
> 
> I can commit it if you're fine with the change.
thanks for reporting this!

sent out rG3346cecd4c0c960377b441606b6382a684daf061


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71248



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

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

2019-12-19 Thread Alexandre Ganea via Phabricator via cfe-commits

aganea added inline comments.



Comment at: clang-tools-extra/clangd/FormattedString.cpp:110
+// Separate from next block.
+*WritePtr++ = ' ';
+  }

There's an issue at this line, the iterator might go past the end of the string 
(if running the `Document.Separators` test). This fails when running a Debug 
build with VS 2019. The code fails with this assert (in `xstring`):
```
_STL_VERIFY(_Unfancy(_Ptr) < _Mycont->_Myptr() + _Mycont->_Mysize, "cannot 
increment string iterator past end");
```
I changed the code such as: {F11108172}
(sorry, Phabricator throws an exception when uploading this patch as a 
differential)

I can commit it if you're fine with the change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71248



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

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

2019-12-13 Thread Kadir Cetinkaya via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG597c6b65552a: [clangd] Introduce paragraph, the first part 
of new rendering structs (authored by kadircet).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71248

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

Index: clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
===
--- clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
+++ clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
@@ -8,192 +8,109 @@
 #include "FormattedString.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/StringRef.h"
-
+#include "llvm/Support/raw_ostream.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
+namespace markup {
 namespace {
 
-TEST(FormattedString, Basic) {
-  FormattedString S;
-  EXPECT_EQ(S.renderAsPlainText(), "");
-  EXPECT_EQ(S.renderAsMarkdown(), "");
-
-  S.appendText("foobar  ");
-  S.appendText("baz");
-  EXPECT_EQ(S.renderAsPlainText(), "foobar baz");
-  EXPECT_EQ(S.renderAsMarkdown(), "foobar  baz");
-
-  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");
-  S.appendText("after");
-
-  std::string ExpectedText = R"(foobar
-
-bazqux
-
-after)";
-  EXPECT_EQ(S.renderAsPlainText(), ExpectedText);
-  std::string ExpectedMarkdown = R"md(```cpp
-foobar
-```
-```javascript
-bazqux
-```
-after)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) {
+TEST(Render, Escaping) {
   // Check some ASCII punctuation
-  FormattedString S;
-  S.appendText("*!`");
-  EXPECT_EQ(S.renderAsMarkdown(), "\\*\\!\\`");
+  Paragraph P;
+  P.appendText("*!`");
+  EXPECT_EQ(P.asMarkdown(), "\\*\\!\\`");
 
   // Check all ASCII punctuation.
-  S = FormattedString();
+  P = Paragraph();
   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);
+  P.appendText(Punctuation);
+  EXPECT_EQ(P.asMarkdown(), 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");
+  P = Paragraph();
+  P.appendCode("* foo !+ bar * baz");
+  EXPECT_EQ(P.asMarkdown(), "`* foo !+ bar * baz`");
 
   // 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");
+  P = Paragraph();
+  P.appendCode("foo`bar`baz");
+  EXPECT_EQ(P.asMarkdown(), "`foo``bar``baz`");
 
   // 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

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

2019-12-12 Thread pre-merge checks [bot] via Phabricator via cfe-commits

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 60763 tests passed, 0 failed 
and 726 were skipped.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
console-log.txt 
,
 CMakeCache.txt 
,
 test-results.xml 
,
 diff.json 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71248



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

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

2019-12-12 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 233576.
kadircet marked 5 inline comments as done.
kadircet added a comment.

- Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71248

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

Index: clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
===
--- clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
+++ clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
@@ -8,192 +8,109 @@
 #include "FormattedString.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/StringRef.h"
-
+#include "llvm/Support/raw_ostream.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
+namespace markup {
 namespace {
 
-TEST(FormattedString, Basic) {
-  FormattedString S;
-  EXPECT_EQ(S.renderAsPlainText(), "");
-  EXPECT_EQ(S.renderAsMarkdown(), "");
-
-  S.appendText("foobar  ");
-  S.appendText("baz");
-  EXPECT_EQ(S.renderAsPlainText(), "foobar baz");
-  EXPECT_EQ(S.renderAsMarkdown(), "foobar  baz");
-
-  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");
-  S.appendText("after");
-
-  std::string ExpectedText = R"(foobar
-
-bazqux
-
-after)";
-  EXPECT_EQ(S.renderAsPlainText(), ExpectedText);
-  std::string ExpectedMarkdown = R"md(```cpp
-foobar
-```
-```javascript
-bazqux
-```
-after)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) {
+TEST(Render, Escaping) {
   // Check some ASCII punctuation
-  FormattedString S;
-  S.appendText("*!`");
-  EXPECT_EQ(S.renderAsMarkdown(), "\\*\\!\\`");
+  Paragraph P;
+  P.appendText("*!`");
+  EXPECT_EQ(P.asMarkdown(), "\\*\\!\\`");
 
   // Check all ASCII punctuation.
-  S = FormattedString();
+  P = Paragraph();
   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);
+  P.appendText(Punctuation);
+  EXPECT_EQ(P.asMarkdown(), 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");
+  P = Paragraph();
+  P.appendCode("* foo !+ bar * baz");
+  EXPECT_EQ(P.asMarkdown(), "`* foo !+ bar * baz`");
 
   // 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");
+  P = Paragraph();
+  P.appendCode("foo`bar`baz");
+  EXPECT_EQ(P.asMarkdown(), "`foo``bar``baz`");
 
   // 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] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

2019-12-12 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/unittests/FormattedStringTests.cpp:79
+  {
+  Test::PlainText,
+  "after",

sammccall wrote:
> I'd consider writing this as
> `[&] { Para.appendText("after"); }` to make it clearer what's being tested.
> 
> Are you sure the table test is clearer than straight-line code incrementally 
> building the paragrap/asserting here?
this test was huge in the beginning and having a straight line of appends were 
making it really hard to reason with. but it seems leaner now, as it doesn't 
have codeblocks anymore.
switching back to linear testing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71248



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

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

2019-12-12 Thread 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.cpp:164
   }
-  return R;
+  OS << '\n';
 }

this is worth a comment - we translate Paragraphs into markdown lines, not 
markdown paragraphs



Comment at: clang-tools-extra/clangd/FormattedString.cpp:199
+Paragraph ::addParagraph() {
+  Children.emplace_back(std::make_unique());
+  return *static_cast(Children.back().get());

push_back (and below)



Comment at: clang-tools-extra/clangd/unittests/FormattedStringTests.cpp:21
 
-TEST(FormattedString, Basic) {
-  FormattedString S;
-  EXPECT_EQ(S.renderAsPlainText(), "");
-  EXPECT_EQ(S.renderAsMarkdown(), "");
+enum RenderKind {
+  PlainText,

dead?



Comment at: clang-tools-extra/clangd/unittests/FormattedStringTests.cpp:79
+  {
+  Test::PlainText,
+  "after",

I'd consider writing this as
`[&] { Para.appendText("after"); }` to make it clearer what's being tested.

Are you sure the table test is clearer than straight-line code incrementally 
building the paragrap/asserting here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71248



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

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

2019-12-12 Thread pre-merge checks [bot] via Phabricator via cfe-commits

merge_guards_bot added a comment.

{icon times-circle color=red} Unit tests: fail. 60762 tests passed, 1 failed 
and 726 were skipped.

  failed: Clang.CodeGenCXX/runtime-dllstorage.cpp

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
console-log.txt 
,
 CMakeCache.txt 
,
 test-results.xml 
,
 diff.json 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71248



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

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

2019-12-12 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet marked 12 inline comments as done.
kadircet added inline comments.



Comment at: clang-tools-extra/clangd/FormattedString.cpp:122
+};
+std::string renderBlocks(llvm::ArrayRef> Children,
+ RenderType RT) {

sammccall wrote:
> I'm not sure this common codepath for plain-text and markdown will survive in 
> the long run:
>  - state around indentation and wrapping is likely to differ
>  - separating blocks with one newline may not be the right thing in each case
I suppose this will survive in the long run now, as it only prints blocks, 
without separation, and trims in the end.



Comment at: clang-tools-extra/clangd/FormattedString.cpp:130
+OS << Sep;
+Sep = "\n";
+((*C).*RenderFunc)(OS);

sammccall wrote:
> for markdown, this means consecutive paragraphs will be rendered as a single 
> paragraph with line breaks in it. intended?
yes this was intended, but as each block takes care of its own indentation now, 
this is no longer an issue.



Comment at: clang-tools-extra/clangd/FormattedString.h:29
 public:
+  virtual void asMarkdown(llvm::raw_ostream ) const = 0;
+  virtual void asPlainText(llvm::raw_ostream ) const = 0;

sammccall wrote:
> Do these include trailing newline(s), or not?
> 
> Based on offline discussion, they currently do not, but this causes some 
> problems e.g. list followed by paragraph would get merged.
> 
> Option a) have Blocks include required trailing newlines (e.g. \n\n after 
> lists in markdown, \n after paragraphs), and have Document trim trailing 
> newlines.
> Option b) have Document look at the type of the elements above/below and 
> insert space appropriately.
> 
> Personally prefer option A because it encapsulates behavior better in the 
> classes (less dyn_cast) and I think reflects the grammar better.
taking option A



Comment at: clang-tools-extra/clangd/Hover.cpp:460
   if (!Definition.empty()) {
-Output.appendCodeBlock(Definition);
+Output.addParagraph().appendCode(Definition);
   } else {

sammccall wrote:
> This is a regression at the moment - complicated definitions are 
> clang-formatted, and we'll canonicalize the whitespace.
> (The behavior of the library is correct, we just need code block for this).
not planning to land until we've got the complete implementation, including 
lists and codeblocks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71248



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

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

2019-12-12 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 233566.
kadircet marked 4 inline comments as done.
kadircet added a comment.

- Move separation logic from container to blocks
- Add two concrete APIs to block for getting strings and update tests to use 
those.
- Rename methods


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71248

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

Index: clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
===
--- clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
+++ clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
@@ -8,192 +8,143 @@
 #include "FormattedString.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/raw_ostream.h"
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
+namespace markup {
 namespace {
 
-TEST(FormattedString, Basic) {
-  FormattedString S;
-  EXPECT_EQ(S.renderAsPlainText(), "");
-  EXPECT_EQ(S.renderAsMarkdown(), "");
+enum RenderKind {
+  PlainText,
+  Markdown,
+};
 
-  S.appendText("foobar  ");
-  S.appendText("baz");
-  EXPECT_EQ(S.renderAsPlainText(), "foobar baz");
-  EXPECT_EQ(S.renderAsMarkdown(), "foobar  baz");
-
-  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");
-  S.appendText("after");
-
-  std::string ExpectedText = R"(foobar
-
-bazqux
-
-after)";
-  EXPECT_EQ(S.renderAsPlainText(), ExpectedText);
-  std::string ExpectedMarkdown = R"md(```cpp
-foobar
-```
-```javascript
-bazqux
-```
-after)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) {
+TEST(Render, Escaping) {
   // Check some ASCII punctuation
-  FormattedString S;
-  S.appendText("*!`");
-  EXPECT_EQ(S.renderAsMarkdown(), "\\*\\!\\`");
+  Paragraph P;
+  P.appendText("*!`");
+  EXPECT_EQ(P.asMarkdown(), "\\*\\!\\`");
 
   // Check all ASCII punctuation.
-  S = FormattedString();
+  P = Paragraph();
   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);
+  P.appendText(Punctuation);
+  EXPECT_EQ(P.asMarkdown(), 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");
+  P = Paragraph();
+  P.appendCode("* foo !+ bar * baz");
+  EXPECT_EQ(P.asMarkdown(), "`* foo !+ bar * baz`");
 
   // 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");
+  P = Paragraph();
+  P.appendCode("foo`bar`baz");
+  EXPECT_EQ(P.asMarkdown(), "`foo``bar``baz`");
 
   // 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();
-

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

2019-12-12 Thread Sam McCall via Phabricator via cfe-commits

sammccall added a comment.

Design question around newlines :-) Otherwise looks good.

Comment at: clang-tools-extra/clangd/FormattedString.cpp:95
+// Concatanates whitespace blocks into a single ` `.
+std::string canonicalizeSpaces(std::string Input) {
+  // Goes over the string and preserves only a single ` ` for any whitespace

kadircet wrote:
> sammccall wrote:
> > is this equivalent to
> > ```
> > SmallVector Words;
> > llvm::SplitString(Input, Words);
> > return llvm::join(Input, " ");
> > ```
> > ?
> > 
> > (That would trim leading/trailing whitespace, but I suspect we actually 
> > want that)
> yes that's exactly what we want, thanks!
> not just joining them though, since we could still merge them in-place.
I'm really not sure saving the allocation is worth the code here, but up to you.

Comment at: clang-tools-extra/clangd/FormattedString.cpp:118

-std::string FormattedString::renderAsMarkdown() const {
+enum RenderType {
+  PlainText,

(why not just pass in the fptr?)

Comment at: clang-tools-extra/clangd/FormattedString.cpp:122
+};
+std::string renderBlocks(llvm::ArrayRef> Children,
+ RenderType RT) {

I'm not sure this common codepath for plain-text and markdown will survive in 
the long run:
 - state around indentation and wrapping is likely to differ
 - separating blocks with one newline may not be the right thing in each case

Comment at: clang-tools-extra/clangd/FormattedString.cpp:130
+OS << Sep;
+Sep = "\n";
+((*C).*RenderFunc)(OS);

for markdown, this means consecutive paragraphs will be rendered as a single 
paragraph with line breaks in it. intended?

Comment at: clang-tools-extra/clangd/FormattedString.cpp:139
+public:
+  void asMarkdown(llvm::raw_ostream ) const override {}
+  void asPlainText(llvm::raw_ostream ) const override {}

this deserves a comment - the effect is produced by the surrounding newlines.
But if we change the blocks to include their whitespace it'll be more obvious.

Comment at: clang-tools-extra/clangd/FormattedString.h:29
 public:
+  virtual void asMarkdown(llvm::raw_ostream ) const = 0;
+  virtual void asPlainText(llvm::raw_ostream ) const = 0;

Do these include trailing newline(s), or not?

Based on offline discussion, they currently do not, but this causes some 
problems e.g. list followed by paragraph would get merged.

Option a) have Blocks include required trailing newlines (e.g. \n\n after lists 
in markdown, \n after paragraphs), and have Document trim trailing newlines.
Option b) have Document look at the type of the elements above/below and insert 
space appropriately.

Personally prefer option A because it encapsulates behavior better in the 
classes (less dyn_cast) and I think reflects the grammar better.

Comment at: clang-tools-extra/clangd/Hover.cpp:460
   if (!Definition.empty()) {
-Output.appendCodeBlock(Definition);
+Output.addParagraph().appendCode(Definition);
   } else {

This is a regression at the moment - complicated definitions are 
clang-formatted, and we'll canonicalize the whitespace.
(The behavior of the library is correct, we just need code block for this).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71248

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

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

2019-12-11 Thread pre-merge checks [bot] via Phabricator via cfe-commits

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 60657 tests passed, 0 failed 
and 726 were skipped.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
console-log.txt 
,
 CMakeCache.txt 
,
 test-results.xml 
,
 diff.json 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71248



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

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

2019-12-11 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 233513.
kadircet added a comment.

- More tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71248

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

Index: clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
===
--- clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
+++ clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
@@ -8,192 +8,159 @@
 #include "FormattedString.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/raw_ostream.h"
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
+namespace markup {
 namespace {
 
-TEST(FormattedString, Basic) {
-  FormattedString S;
-  EXPECT_EQ(S.renderAsPlainText(), "");
-  EXPECT_EQ(S.renderAsMarkdown(), "");
-
-  S.appendText("foobar  ");
-  S.appendText("baz");
-  EXPECT_EQ(S.renderAsPlainText(), "foobar baz");
-  EXPECT_EQ(S.renderAsMarkdown(), "foobar  baz");
-
-  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");
+enum RenderKind {
+  PlainText,
+  Markdown,
+};
+std::string renderBlock(const Block , RenderKind RK) {
+  std::string R;
+  llvm::raw_string_ostream OS(R);
+  switch (RK) {
+  case PlainText:
+B.asPlainText(OS);
+break;
+  case Markdown:
+B.asMarkdown(OS);
+break;
+  }
+  return OS.str();
 }
 
-TEST(FormattedString, CodeBlocks) {
-  FormattedString S;
-  S.appendCodeBlock("foobar");
-  S.appendCodeBlock("bazqux", "javascript");
-  S.appendText("after");
-
-  std::string ExpectedText = R"(foobar
-
-bazqux
-
-after)";
-  EXPECT_EQ(S.renderAsPlainText(), ExpectedText);
-  std::string ExpectedMarkdown = R"md(```cpp
-foobar
-```
-```javascript
-bazqux
-```
-after)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");
-}
+MATCHER_P(HasMarkdown, MD, "") { return renderBlock(arg, Markdown) == MD; }
+MATCHER_P(HasPlainText, PT, "") { return renderBlock(arg, PlainText) == PT; }
 
-TEST(FormattedString, Escaping) {
+TEST(Render, Escaping) {
   // Check some ASCII punctuation
-  FormattedString S;
-  S.appendText("*!`");
-  EXPECT_EQ(S.renderAsMarkdown(), "\\*\\!\\`");
+  Paragraph P;
+  P.appendText("*!`");
+  EXPECT_THAT(P, HasMarkdown("\\*\\!\\`"));
 
   // Check all ASCII punctuation.
-  S = FormattedString();
+  P = Paragraph();
   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);
+  P.appendText(Punctuation);
+  EXPECT_THAT(P, HasMarkdown(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");
+  P = Paragraph();
+  P.appendCode("* foo !+ bar * baz");
+  EXPECT_THAT(P, HasMarkdown("`* foo !+ bar * baz`"));
 
   // 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");
+  P = Paragraph();
+  P.appendCode("foo`bar`baz");
+  EXPECT_THAT(P, HasMarkdown("`foo``bar``baz`"));
 
   // Inline code blocks starting or ending with backticks should add spaces.

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

2019-12-11 Thread pre-merge checks [bot] via Phabricator via cfe-commits

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 60655 tests passed, 0 failed 
and 726 were skipped.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
console-log.txt 
,
 CMakeCache.txt 
,
 test-results.xml 
,
 diff.json 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71248



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

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

2019-12-11 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 233420.
kadircet marked 12 inline comments as done.
kadircet added a comment.

- Move code blocks out of Paragraph
- Provide a way to control vertical spacing
- Address comments around naming


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71248

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

Index: clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
===
--- clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
+++ clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
@@ -8,192 +8,139 @@
 #include "FormattedString.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/raw_ostream.h"
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
+namespace markup {
 namespace {
 
-TEST(FormattedString, Basic) {
-  FormattedString S;
-  EXPECT_EQ(S.renderAsPlainText(), "");
-  EXPECT_EQ(S.renderAsMarkdown(), "");
-
-  S.appendText("foobar  ");
-  S.appendText("baz");
-  EXPECT_EQ(S.renderAsPlainText(), "foobar baz");
-  EXPECT_EQ(S.renderAsMarkdown(), "foobar  baz");
-
-  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");
+enum RenderKind {
+  PlainText,
+  Markdown,
+};
+std::string renderBlock(const Block , RenderKind RK) {
+  std::string R;
+  llvm::raw_string_ostream OS(R);
+  switch (RK) {
+  case PlainText:
+B.asPlainText(OS);
+break;
+  case Markdown:
+B.asMarkdown(OS);
+break;
+  }
+  return OS.str();
 }
 
-TEST(FormattedString, CodeBlocks) {
-  FormattedString S;
-  S.appendCodeBlock("foobar");
-  S.appendCodeBlock("bazqux", "javascript");
-  S.appendText("after");
-
-  std::string ExpectedText = R"(foobar
-
-bazqux
-
-after)";
-  EXPECT_EQ(S.renderAsPlainText(), ExpectedText);
-  std::string ExpectedMarkdown = R"md(```cpp
-foobar
-```
-```javascript
-bazqux
-```
-after)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");
-}
+MATCHER_P(HasMarkdown, MD, "") { return renderBlock(arg, Markdown) == MD; }
+MATCHER_P(HasPlainText, PT, "") { return renderBlock(arg, PlainText) == PT; }
 
-TEST(FormattedString, Escaping) {
+TEST(Render, Escaping) {
   // Check some ASCII punctuation
-  FormattedString S;
-  S.appendText("*!`");
-  EXPECT_EQ(S.renderAsMarkdown(), "\\*\\!\\`");
+  Paragraph P;
+  P.appendText("*!`");
+  EXPECT_THAT(P, HasMarkdown("\\*\\!\\`"));
 
   // Check all ASCII punctuation.
-  S = FormattedString();
+  P = Paragraph();
   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);
+  P.appendText(Punctuation);
+  EXPECT_THAT(P, HasMarkdown(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");
+  P = Paragraph();
+  P.appendCode("* foo !+ bar * baz");
+  EXPECT_THAT(P, HasMarkdown("`* foo !+ bar * baz`"));
 
   // 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");
+  P = Paragraph();
+

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

2019-12-11 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/FormattedString.cpp:95
+// Concatanates whitespace blocks into a single ` `.
+std::string canonicalizeSpaces(std::string Input) {
+  // Goes over the string and preserves only a single ` ` for any whitespace

sammccall wrote:
> is this equivalent to
> ```
> SmallVector Words;
> llvm::SplitString(Input, Words);
> return llvm::join(Input, " ");
> ```
> ?
> 
> (That would trim leading/trailing whitespace, but I suspect we actually want 
> that)
yes that's exactly what we want, thanks!
not just joining them though, since we could still merge them in-place.



Comment at: clang-tools-extra/clangd/FormattedString.h:30
+  virtual std::string renderAsPlainText() const = 0;
+
+  virtual ~RenderableBlock() = default;

sammccall wrote:
> renderfortests has gone. What's the plan for testing Hover rendering? (i.e. 
> HoverInfo -> Document)
> I guess asserting the plain text and markdown might be enough...
> 
> (Did we remove *all* the tests for `present()`in D70911, and I didn't notice? 
> We should restore some based on synthetic HoverInfo)
yes we I was planning to check using the plaintext output with some "synthetic" 
hoverinfo. Currently only lit tests are checking for the behavior of present.
there's also changes to hover.cpp in this patch to preserve current output. 
happy to add some tests before moving forward, but didn't want to add any as 
they
will change drastically with the new rendering layout.



Comment at: clang-tools-extra/clangd/FormattedString.h:35
+/// Represents parts of the markup that can contain strings, like inline code,
+/// code block or plain text.
+/// Only CodeBlocks guarantees conservation of new lines within text. One must

sammccall wrote:
> A code block doesn't go in a paragraph, as it's a block itself.
> 
> "A paragraph contains a logical line of rich text. It may be wrapped for 
> display"
sending out codeblocks in a new patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71248



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

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

2019-12-11 Thread Sam McCall via Phabricator via cfe-commits

sammccall added a comment.

Main thing here is I'm fairly sure that code blocks shouldn't be chunks in a 
paragraph (or I'm misunderstanding what a paragraph is)




Comment at: clang-tools-extra/clangd/FormattedString.cpp:95
+// Concatanates whitespace blocks into a single ` `.
+std::string canonicalizeSpaces(std::string Input) {
+  // Goes over the string and preserves only a single ` ` for any whitespace

is this equivalent to
```
SmallVector Words;
llvm::SplitString(Input, Words);
return llvm::join(Input, " ");
```
?

(That would trim leading/trailing whitespace, but I suspect we actually want 
that)



Comment at: clang-tools-extra/clangd/FormattedString.cpp:176
   std::string R;
-  for (const auto  : Chunks) {
+  auto EnsureWhitespace = []() {
+// Adds a space for nicer rendering.

Can we simplify the whitespace model? A lot of this is hacks around code 
blocks, which shouldn't really be here.

What about:
 - the contents of each chunk never have leading/trailing ws (it's stripped 
when adding)
 - there's always a space between consecutive chunks



Comment at: clang-tools-extra/clangd/FormattedString.h:24
 
 /// A structured string representation that could be converted to markdown or
 /// plaintext upon requrest.

I don't think this comment captures what this class is about now. (Rewriting 
the class but leaving the comment intact is suspicious!)

Comment should allude to how blocks make up a document - a RenderableBlock 
holds rich text and knows how to lay it out. Blocks can be stacked to form a 
document, etc.



Comment at: clang-tools-extra/clangd/FormattedString.h:26
 /// plaintext upon requrest.
-class FormattedString {
+class RenderableBlock {
 public:

nit: I wouldn't bother with both "renderable" prefix and the `markup` 
namespace, up to you



Comment at: clang-tools-extra/clangd/FormattedString.h:28
 public:
+  virtual std::string renderAsMarkdown() const = 0;
+  virtual std::string renderAsPlainText() const = 0;

Or just asMarkdown, up to you



Comment at: clang-tools-extra/clangd/FormattedString.h:28
 public:
+  virtual std::string renderAsMarkdown() const = 0;
+  virtual std::string renderAsPlainText() const = 0;

sammccall wrote:
> Or just asMarkdown, up to you
are you sure we want to create all the temporary strings?
Consider `void renderMarkdown(llvm::raw_ostream&)`, and keep `std::string 
asMarkdown()` on `Document`



Comment at: clang-tools-extra/clangd/FormattedString.h:30
+  virtual std::string renderAsPlainText() const = 0;
+
+  virtual ~RenderableBlock() = default;

renderfortests has gone. What's the plan for testing Hover rendering? (i.e. 
HoverInfo -> Document)
I guess asserting the plain text and markdown might be enough...

(Did we remove *all* the tests for `present()`in D70911, and I didn't notice? 
We should restore some based on synthetic HoverInfo)



Comment at: clang-tools-extra/clangd/FormattedString.h:35
+/// Represents parts of the markup that can contain strings, like inline code,
+/// code block or plain text.
+/// Only CodeBlocks guarantees conservation of new lines within text. One must

A code block doesn't go in a paragraph, as it's a block itself.

"A paragraph contains a logical line of rich text. It may be wrapped for 
display"



Comment at: clang-tools-extra/clangd/FormattedString.h:46
+
   /// Append an inline block of C++ code. This translates to the ` block in
   /// markdown.

nit: while here, I do think `inline block` is misleading - this corresponds to 
display: inline, not display:inline-block. "inline span"?



Comment at: clang-tools-extra/clangd/FormattedString.h:48
   /// markdown.
-  void appendInlineCode(std::string Code);
+  Paragraph (std::string Code);
 

just appendCode?



Comment at: clang-tools-extra/clangd/FormattedString.h:69
 
+/// A semantical representation for formattable strings. Allows rendering into
+/// markdown and plaintext.

semantic -> format-agnostic? This doesn't capture semantics.

formattable strings -> rich text, or structured text?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71248



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

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

2019-12-10 Thread pre-merge checks [bot] via Phabricator via cfe-commits

merge_guards_bot added a comment.

Build result: pass - 60655 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71248



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

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

2019-12-10 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 233078.
kadircet added a comment.

- More comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71248

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

Index: clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
===
--- clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
+++ clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
@@ -14,186 +14,233 @@
 
 namespace clang {
 namespace clangd {
+namespace markup {
 namespace {
 
-TEST(FormattedString, Basic) {
-  FormattedString S;
-  EXPECT_EQ(S.renderAsPlainText(), "");
-  EXPECT_EQ(S.renderAsMarkdown(), "");
-
-  S.appendText("foobar  ");
-  S.appendText("baz");
-  EXPECT_EQ(S.renderAsPlainText(), "foobar baz");
-  EXPECT_EQ(S.renderAsMarkdown(), "foobar  baz");
-
-  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");
-  S.appendText("after");
-
-  std::string ExpectedText = R"(foobar
-
-bazqux
-
-after)";
-  EXPECT_EQ(S.renderAsPlainText(), ExpectedText);
-  std::string ExpectedMarkdown = R"md(```cpp
-foobar
-```
-```javascript
-bazqux
-```
-after)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) {
+TEST(Render, Escaping) {
   // Check some ASCII punctuation
-  FormattedString S;
-  S.appendText("*!`");
-  EXPECT_EQ(S.renderAsMarkdown(), "\\*\\!\\`");
+  Paragraph P;
+  P.appendText("*!`");
+  EXPECT_EQ(P.renderAsMarkdown(), "\\*\\!\\`");
 
   // Check all ASCII punctuation.
-  S = FormattedString();
+  P = Paragraph();
   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);
+  P.appendText(Punctuation);
+  EXPECT_EQ(P.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"
+  P = Paragraph();
+  P.appendInlineCode("* foo !+ bar * baz");
+  EXPECT_EQ(P.renderAsMarkdown(), "`* foo !+ bar * baz`");
+  P = Paragraph();
+  P.appendCodeBlock("#define FOO\n* foo !+ bar * baz");
+  EXPECT_EQ(P.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`");
+  P = Paragraph();
+  P.appendInlineCode("foo`bar`baz");
+  EXPECT_EQ(P.renderAsMarkdown(), "`foo``bar``baz`");
 
-  S = FormattedString();
-  S.appendCodeBlock("foo`bar`baz");
-  EXPECT_EQ(S.renderAsMarkdown(), "```cpp\n"
+  P = Paragraph();
+  P.appendCodeBlock("foo`bar`baz");
+  EXPECT_EQ(P.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

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

2019-12-10 Thread pre-merge checks [bot] via Phabricator via cfe-commits

merge_guards_bot added a comment.

Build result: pass - 60655 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71248



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

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

2019-12-10 Thread Kadir Cetinkaya via Phabricator via cfe-commits

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

Initial patch for new rendering structs in clangd.

Splitting implementation into smaller chunks, for a full view of the API see 
D71063 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71248

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

Index: clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
===
--- clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
+++ clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
@@ -14,186 +14,233 @@
 
 namespace clang {
 namespace clangd {
+namespace markup {
 namespace {
 
-TEST(FormattedString, Basic) {
-  FormattedString S;
-  EXPECT_EQ(S.renderAsPlainText(), "");
-  EXPECT_EQ(S.renderAsMarkdown(), "");
-
-  S.appendText("foobar  ");
-  S.appendText("baz");
-  EXPECT_EQ(S.renderAsPlainText(), "foobar baz");
-  EXPECT_EQ(S.renderAsMarkdown(), "foobar  baz");
-
-  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");
-  S.appendText("after");
-
-  std::string ExpectedText = R"(foobar
-
-bazqux
-
-after)";
-  EXPECT_EQ(S.renderAsPlainText(), ExpectedText);
-  std::string ExpectedMarkdown = R"md(```cpp
-foobar
-```
-```javascript
-bazqux
-```
-after)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) {
+TEST(Render, Escaping) {
   // Check some ASCII punctuation
-  FormattedString S;
-  S.appendText("*!`");
-  EXPECT_EQ(S.renderAsMarkdown(), "\\*\\!\\`");
+  Paragraph P;
+  P.appendText("*!`");
+  EXPECT_EQ(P.renderAsMarkdown(), "\\*\\!\\`");
 
   // Check all ASCII punctuation.
-  S = FormattedString();
+  P = Paragraph();
   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);
+  P.appendText(Punctuation);
+  EXPECT_EQ(P.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"
+  P = Paragraph();
+  P.appendInlineCode("* foo !+ bar * baz");
+  EXPECT_EQ(P.renderAsMarkdown(), "`* foo !+ bar * baz`");
+  P = Paragraph();
+  P.appendCodeBlock("#define FOO\n* foo !+ bar * baz");
+  EXPECT_EQ(P.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`");
+  P = Paragraph();
+  P.appendInlineCode("foo`bar`baz");
+  EXPECT_EQ(P.renderAsMarkdown(), "`foo``bar``baz`");
 
-  S = FormattedString();
-  S.appendCodeBlock("foo`bar`baz");
-  EXPECT_EQ(S.renderAsMarkdown(), "```cpp\n"
+  P = Paragraph();
+  P.appendCodeBlock("foo`bar`baz");
+  EXPECT_EQ(P.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`");
-

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

21 matches

Site Navigation

Mail list logo

Footer information