Skip to content

Commit

Permalink
[clangd] Fix whitespace between chunks in markdown paragraphs.
Browse files Browse the repository at this point in the history 
Summary:
Old model: chunks are always separated by one space.
           This makes it impossible to render "Foo `bar`." correctly.

New model: chunks are separated by space if the left had trailing space, or
           the right had leading space, or space was explicitly requested.
           (Only leading/trailing space in plaintext chunks count, not code)

Reviewers: kadircet

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D79139
  • Loading branch information
@sam-mccall
sam-mccall committed May 2, 2020
1 parent 030ff90 commit ec170b7
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 22 deletions.
31 changes: 24 additions & 7 deletions clang-tools-extra/clangd/FormattedString.cpp
View file
Expand Up @@ -346,18 +346,21 @@ std::string Block::asPlainText() const {
}

void Paragraph::renderMarkdown(llvm::raw_ostream &OS) const {
llvm::StringRef Sep = "";
bool NeedsSpace = false;
bool HasChunks = false;
for (auto &C : Chunks) {
OS << Sep;
if (C.SpaceBefore || NeedsSpace)
OS << " ";
switch (C.Kind) {
case Chunk::PlainText:
OS << renderText(C.Contents, Sep.empty());
OS << renderText(C.Contents, !HasChunks);
break;
case Chunk::InlineCode:
OS << renderInlineBlock(C.Contents);
break;
}
Sep = " ";
HasChunks = true;
NeedsSpace = C.SpaceAfter;
}
// Paragraphs are translated into markdown lines, not markdown paragraphs.
// Therefore it only has a single linebreak afterwards.
Expand All @@ -381,13 +384,15 @@ llvm::StringRef chooseMarker(llvm::ArrayRef<llvm::StringRef> Options,
}

void Paragraph::renderPlainText(llvm::raw_ostream &OS) const {
llvm::StringRef Sep = "";
bool NeedsSpace = false;
for (auto &C : Chunks) {
if (C.SpaceBefore || NeedsSpace)
OS << " ";
llvm::StringRef Marker = "";
if (C.Preserve && C.Kind == Chunk::InlineCode)
Marker = chooseMarker({"`", "'", "\""}, C.Contents);
OS << Sep << Marker << C.Contents << Marker;
Sep = " ";
OS << Marker << C.Contents << Marker;
NeedsSpace = C.SpaceAfter;
}
OS << '\n';
}
Expand All @@ -410,6 +415,12 @@ void BulletList::renderPlainText(llvm::raw_ostream &OS) const {
}
}

Paragraph &Paragraph::appendSpace() {
if (!Chunks.empty())
Chunks.back().SpaceAfter = true;
return *this;
}

Paragraph &Paragraph::appendText(llvm::StringRef Text) {
std::string Norm = canonicalizeSpaces(Text);
if (Norm.empty())
Expand All @@ -418,10 +429,14 @@ Paragraph &Paragraph::appendText(llvm::StringRef Text) {
Chunk &C = Chunks.back();
C.Contents = std::move(Norm);
C.Kind = Chunk::PlainText;
C.SpaceBefore = isWhitespace(Text.front());
C.SpaceAfter = isWhitespace(Text.back());
return *this;
}

Paragraph &Paragraph::appendCode(llvm::StringRef Code, bool Preserve) {
bool AdjacentCode =
!Chunks.empty() && Chunks.back().Kind == Chunk::InlineCode;
std::string Norm = canonicalizeSpaces(std::move(Code));
if (Norm.empty())
return *this;
Expand All @@ -430,6 +445,8 @@ Paragraph &Paragraph::appendCode(llvm::StringRef Code, bool Preserve) {
C.Contents = std::move(Norm);
C.Kind = Chunk::InlineCode;
C.Preserve = Preserve;
// Disallow adjacent code spans without spaces, markdown can't render them.
C.SpaceBefore = AdjacentCode;
return *this;
}

Expand Down
9 changes: 9 additions & 0 deletions clang-tools-extra/clangd/FormattedString.h
View file
Expand Up @@ -54,6 +54,10 @@ class Paragraph : public Block {
/// \p Preserve indicates the code span must be apparent even in plaintext.
Paragraph &appendCode(llvm::StringRef Code, bool Preserve = false);

/// Ensure there is space between the surrounding chunks.
/// Has no effect at the beginning or end of a paragraph.
Paragraph &appendSpace();

private:
struct Chunk {
enum {
Expand All @@ -63,6 +67,11 @@ class Paragraph : public Block {
// Preserve chunk markers in plaintext.
bool Preserve = false;
std::string Contents;
// Whether this chunk should be surrounded by whitespace.
// Consecutive SpaceAfter and SpaceBefore will be collapsed into one space.
// Code spans don't usually set this: their spaces belong "inside" the span.
bool SpaceBefore = false;
bool SpaceAfter = false;
};
std::vector<Chunk> Chunks;
};
Expand Down
10 changes: 5 additions & 5 deletions clang-tools-extra/clangd/Hover.cpp
View file
Expand Up @@ -773,7 +773,7 @@ markup::Document HoverInfo::present() const {
// https://github.com/microsoft/vscode/issues/88417 for details.
markup::Paragraph &Header = Output.addHeading(3);
if (Kind != index::SymbolKind::Unknown)
Header.appendText(index::getSymbolKindString(Kind));
Header.appendText(index::getSymbolKindString(Kind)).appendSpace();
assert(!Name.empty() && "hover triggered on a nameless symbol");
Header.appendCode(Name);

Expand All @@ -787,9 +787,9 @@ markup::Document HoverInfo::present() const {
// Parameters:
// - `bool param1`
// - `int param2 = 5`
Output.addParagraph().appendText("→").appendCode(*ReturnType);
Output.addParagraph().appendText("→ ").appendCode(*ReturnType);
if (Parameters && !Parameters->empty()) {
Output.addParagraph().appendText("Parameters:");
Output.addParagraph().appendText("Parameters: ");
markup::BulletList &L = Output.addBulletList();
for (const auto &Param : *Parameters) {
std::string Buffer;
Expand All @@ -804,7 +804,7 @@ markup::Document HoverInfo::present() const {

if (Value) {
markup::Paragraph &P = Output.addParagraph();
P.appendText("Value =");
P.appendText("Value = ");
P.appendCode(*Value);
}

Expand Down Expand Up @@ -883,7 +883,7 @@ void parseDocumentationLine(llvm::StringRef Line, markup::Paragraph &Out) {
break;
}
}
Out.appendText(Line);
Out.appendText(Line).appendSpace();
}

void parseDocumentation(llvm::StringRef Input, markup::Document &Output) {
Expand Down
26 changes: 19 additions & 7 deletions clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
View file
Expand Up @@ -155,11 +155,11 @@ TEST(Paragraph, Chunks) {
Paragraph P = Paragraph();
P.appendText("One ");
P.appendCode("fish");
P.appendText(", two");
P.appendText(", two ");
P.appendCode("fish", /*Preserve=*/true);

EXPECT_EQ(P.asMarkdown(), "One `fish` , two `fish`");
EXPECT_EQ(P.asPlainText(), "One fish , two `fish`");
EXPECT_EQ(P.asMarkdown(), "One `fish`, two `fish`");
EXPECT_EQ(P.asPlainText(), "One fish, two `fish`");
}

TEST(Paragraph, SeparationOfChunks) {
Expand All @@ -168,26 +168,38 @@ TEST(Paragraph, SeparationOfChunks) {
// Purpose is to check for separation between different chunks.
Paragraph P;

P.appendText("after");
P.appendText("after ");
EXPECT_EQ(P.asMarkdown(), "after");
EXPECT_EQ(P.asPlainText(), "after");

P.appendCode("foobar");
P.appendCode("foobar").appendSpace();
EXPECT_EQ(P.asMarkdown(), "after `foobar`");
EXPECT_EQ(P.asPlainText(), "after foobar");

P.appendText("bat");
EXPECT_EQ(P.asMarkdown(), "after `foobar` bat");
EXPECT_EQ(P.asPlainText(), "after foobar bat");

P.appendCode("no").appendCode("space");
EXPECT_EQ(P.asMarkdown(), "after `foobar` bat`no` `space`");
EXPECT_EQ(P.asPlainText(), "after foobar batno space");
}

TEST(Paragraph, ExtraSpaces) {
// Make sure spaces inside chunks are dropped.
Paragraph P;
P.appendText("foo\n \t baz");
P.appendCode(" bar\n");
EXPECT_EQ(P.asMarkdown(), "foo baz `bar`");
EXPECT_EQ(P.asPlainText(), "foo baz bar");
EXPECT_EQ(P.asMarkdown(), "foo baz`bar`");
EXPECT_EQ(P.asPlainText(), "foo bazbar");
}

TEST(Paragraph, SpacesCollapsed) {
Paragraph P;
P.appendText(" foo bar ");
P.appendText(" baz ");
EXPECT_EQ(P.asMarkdown(), "foo bar baz");
EXPECT_EQ(P.asPlainText(), "foo bar baz");
}

TEST(Paragraph, NewLines) {
Expand Down
5 changes: 2 additions & 3 deletions clang-tools-extra/clangd/unittests/HoverTests.cpp
View file
Expand Up @@ -2019,10 +2019,9 @@ TEST(Hover, ParseDocumentation) {
"foo bar",
},
{
// FIXME: we insert spaces between code and text chunk.
"Tests primality of `p`.",
"Tests primality of `p` .",
"Tests primality of `p` .",
"Tests primality of `p`.",
"Tests primality of `p`.",
},
{
"'`' should not occur in `Code`",
Expand Down

0 comments on commit ec170b7

Please sign in to comment.