diff --git a/clang-tools-extra/clangd/FormattedString.h b/clang-tools-extra/clangd/FormattedString.h --- a/clang-tools-extra/clangd/FormattedString.h +++ b/clang-tools-extra/clangd/FormattedString.h @@ -33,6 +33,7 @@ std::string asMarkdown() const; std::string asPlainText() const; + virtual bool isRuler() const { return false; } virtual ~Block() = default; }; @@ -82,8 +83,8 @@ public: /// Adds a semantical block that will be separate from others. Paragraph &addParagraph(); - /// Inserts a vertical space into the document. - void addSpacer(); + /// Inserts a horizontal separator to the document. + void addRuler(); /// Adds a block of code. This translates to a ``` block in markdown. In plain /// text representation, the code block will be surrounded by newlines. void addCodeBlock(std::string Code, std::string Language = "cpp"); diff --git a/clang-tools-extra/clangd/FormattedString.cpp b/clang-tools-extra/clangd/FormattedString.cpp --- a/clang-tools-extra/clangd/FormattedString.cpp +++ b/clang-tools-extra/clangd/FormattedString.cpp @@ -16,6 +16,7 @@ #include "llvm/Support/FormatVariadic.h" #include "llvm/Support/raw_ostream.h" #include +#include #include #include #include @@ -117,16 +118,52 @@ void (Block::*RenderFunc)(llvm::raw_ostream &) const) { std::string R; llvm::raw_string_ostream OS(R); - for (auto &C : Children) + + // Trim rulers. + Children = Children.drop_while( + [](const std::unique_ptr &C) { return C->isRuler(); }); + auto Last = llvm::find_if( + llvm::reverse(Children), + [](const std::unique_ptr &C) { return !C->isRuler(); }); + Children = Children.drop_back(Children.end() - Last.base()); + + bool LastBlockWasRuler = true; + for (const auto &C : Children) { + if (C->isRuler() && LastBlockWasRuler) + continue; + LastBlockWasRuler = C->isRuler(); ((*C).*RenderFunc)(OS); - return llvm::StringRef(OS.str()).trim().str(); + } + + // Get rid of redundant empty lines introduced in plaintext while imitating + // padding in markdown. + std::string AdjustedResult; + llvm::StringRef TrimmedText(OS.str()); + TrimmedText = TrimmedText.trim(); + + llvm::copy_if(TrimmedText, std::back_inserter(AdjustedResult), + [&TrimmedText](const char &C) { + return !llvm::StringRef(TrimmedText.data(), + &C - TrimmedText.data() + 1) + // We allow at most two newlines. + .endswith("\n\n\n"); + }); + + return AdjustedResult; } -// Puts a vertical space between blocks inside a document. -class Spacer : public Block { +// Seperates two blocks with extra spacing. Note that it might render strangely +// in vscode if the trailing block is a codeblock, see +// https://github.com/microsoft/vscode/issues/88416 for details. +class Ruler : public Block { public: - void renderMarkdown(llvm::raw_ostream &OS) const override { OS << '\n'; } + void renderMarkdown(llvm::raw_ostream &OS) const override { + // Note that we need an extra new line before the ruler, otherwise we might + // make previous block a title instead of introducing a ruler. + OS << "\n---\n"; + } void renderPlainText(llvm::raw_ostream &OS) const override { OS << '\n'; } + bool isRuler() const override { return true; } }; class CodeBlock : public Block { @@ -272,7 +309,7 @@ return *static_cast(Children.back().get()); } -void Document::addSpacer() { Children.push_back(std::make_unique()); } +void Document::addRuler() { Children.push_back(std::make_unique()); } void Document::addCodeBlock(std::string Code, std::string Language) { Children.emplace_back( diff --git a/clang-tools-extra/clangd/Hover.cpp b/clang-tools-extra/clangd/Hover.cpp --- a/clang-tools-extra/clangd/Hover.cpp +++ b/clang-tools-extra/clangd/Hover.cpp @@ -532,6 +532,8 @@ Header.appendCode(*Type); } + // Put a linebreak after header to increase readability. + Output.addRuler(); // For functions we display signature in a list form, e.g.: // - `bool param1` // - `int param2 = 5` @@ -555,6 +557,7 @@ Output.addParagraph().appendText(Documentation); if (!Definition.empty()) { + Output.addRuler(); std::string ScopeComment; // Drop trailing "::". if (!LocalScope.empty()) { diff --git a/clang-tools-extra/clangd/unittests/FormattedStringTests.cpp b/clang-tools-extra/clangd/unittests/FormattedStringTests.cpp --- a/clang-tools-extra/clangd/unittests/FormattedStringTests.cpp +++ b/clang-tools-extra/clangd/unittests/FormattedStringTests.cpp @@ -136,13 +136,37 @@ EXPECT_EQ(D.asPlainText(), ExpectedText); } -TEST(Document, Spacer) { +TEST(Document, Ruler) { Document D; D.addParagraph().appendText("foo"); - D.addSpacer(); + D.addRuler(); + + // Ruler followed by paragraph. D.addParagraph().appendText("bar"); - EXPECT_EQ(D.asMarkdown(), "foo \n\nbar"); + EXPECT_EQ(D.asMarkdown(), "foo \n\n---\nbar"); + EXPECT_EQ(D.asPlainText(), "foo\n\nbar"); + + D = Document(); + D.addParagraph().appendText("foo"); + D.addRuler(); + D.addCodeBlock("bar"); + // Ruler followed by a codeblock. + EXPECT_EQ(D.asMarkdown(), "foo \n\n---\n```cpp\nbar\n```"); EXPECT_EQ(D.asPlainText(), "foo\n\nbar"); + + // Ruler followed by another ruler + D = Document(); + D.addParagraph().appendText("foo"); + D.addRuler(); + D.addRuler(); + EXPECT_EQ(D.asMarkdown(), "foo"); + EXPECT_EQ(D.asPlainText(), "foo"); + + // Multiple rulers between blocks + D.addRuler(); + D.addParagraph().appendText("foo"); + EXPECT_EQ(D.asMarkdown(), "foo \n\n---\nfoo"); + EXPECT_EQ(D.asPlainText(), "foo\n\nfoo"); } TEST(Document, Heading) { @@ -182,15 +206,11 @@ foo ```)md"; EXPECT_EQ(D.asMarkdown(), ExpectedMarkdown); - // FIXME: we shouldn't have 2 empty lines in between. A solution might be - // having a `verticalMargin` method for blocks, and let container insert new - // lines according to that before/after blocks. ExpectedPlainText = R"pt(foo bar baz - foo)pt"; EXPECT_EQ(D.asPlainText(), ExpectedPlainText); } diff --git a/clang-tools-extra/clangd/unittests/HoverTests.cpp b/clang-tools-extra/clangd/unittests/HoverTests.cpp --- a/clang-tools-extra/clangd/unittests/HoverTests.cpp +++ b/clang-tools-extra/clangd/unittests/HoverTests.cpp @@ -1699,6 +1699,7 @@ HI.NamespaceScope.emplace(); }, R"(class foo + documentation template class Foo {})", @@ -1722,6 +1723,7 @@ HI.Definition = "ret_type foo(params) {}"; }, R"(function foo → ret_type + - - type - type foo @@ -1740,6 +1742,7 @@ HI.Definition = "def"; }, R"(variable foo : type + Value = value // In test::bar @@ -1765,6 +1768,30 @@ EXPECT_EQ(HI.present().asMarkdown(), "### variable `foo` \\: `type`"); } +// This is a separate test as rulers behave differently in markdown vs +// plaintext. +TEST(Hover, PresentRulers) { + HoverInfo HI; + HI.Kind = index::SymbolKind::Variable; + HI.Name = "foo"; + HI.Value = "val"; + HI.Definition = "def"; + + EXPECT_EQ(HI.present().asMarkdown(), R"md(### variable `foo` + +--- +Value \= `val` + +--- +```cpp +def +```)md"); + EXPECT_EQ(HI.present().asPlainText(), R"pt(variable foo + +Value = val + +def)pt"); +} } // namespace } // namespace clangd } // namespace clang