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

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Dec 12 03:59:30 PST 2019


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 &Document::addParagraph() {
+  Children.emplace_back(std::make_unique<Paragraph>());
+  return *static_cast<Paragraph *>(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





More information about the cfe-commits mailing list