This is an archive of the discontinued LLVM Phabricator instance.

Differential D29943

[clang-format] Align block comment decorations
ClosedPublic

Authored by krasimir on Feb 14 2017, 8:06 AM.

Details

Reviewers
klimek
djasper
bkramer
Commits
rGbb99a36dc004: [clang-format] Align block comment decorations
rC295312: [clang-format] Align block comment decorations
rL295312: [clang-format] Align block comment decorations
Summary

This patch implements block comment decoration alignment.

source:

/* line 1
* line 2
*/

result before:

/* line 1
* line 2
*/

result after:

/* line 1
 * line 2
 */

Diff Detail

Repository
rL LLVM

Event Timeline

krasimir created this revision.Feb 14 2017, 8:06 AM
Herald added a subscriber: klimek. · View Herald TranscriptFeb 14 2017, 8:06 AM
krasimir added reviewers: djasper, bkramer.Feb 14 2017, 8:08 AM
krasimir added a subscriber: cfe-commits.
krasimir added a reviewer: klimek.Feb 15 2017, 1:48 AM
krasimir added a subscriber: mprobst.Feb 16 2017, 2:05 AM
klimek added inline comments.Feb 16 2017, 2:26 AM
lib/Format/BreakableToken.cpp
397 ↗(On Diff #88378)

fewer lines

402 ↗(On Diff #88378)

Why wouldn't we want to align those
/* line
*/
?

krasimir updated this revision to Diff 88713.Feb 16 2017, 4:42 AM
  • Align decorations of block comments spanning two lines
krasimir marked 2 inline comments as done.Feb 16 2017, 4:42 AM
krasimir edited the summary of this revision. (Show Details)
klimek accepted this revision.Feb 16 2017, 4:46 AM

lg

This revision is now accepted and ready to land.Feb 16 2017, 4:46 AM
Closed by commit rL295312: [clang-format] Align block comment decorations (authored by krasimir). · Explain WhyFeb 16 2017, 4:51 AM
This revision was automatically updated to reflect the committed changes.
sylvestre.ledru added a subscriber: sylvestre.ledru.Feb 16 2017, 5:40 AM

Maybe this could be added to the release notes?

krasimir added a comment.Feb 20 2017, 1:42 AM
In D29943#678618, @sylvestre.ledru wrote:

Maybe this could be added to the release notes?

Sounds good! Could you please point me to the release docs? I don't seem to find a clang-format--specific section online.

sylvestre.ledru added a comment.Mar 14 2017, 2:56 AM

I did it for you
Added to the release notes in r297721

Revision Contents

PathSize
cfe/
trunk/
lib/
Format/
4 lines
26 lines
unittests/
Format/
2 lines
202 lines

Diff 88714

cfe/trunk/lib/Format/BreakableToken.h

Show First 20 Lines • Show All 352 Lines • ▼ Show 20 Linesprivate:
unsigned IndentAtLineBreak; unsigned IndentAtLineBreak;
// This is to distinguish between the case when the last line was empty and // This is to distinguish between the case when the last line was empty and
// the case when it started with a decoration ("*" or "* "). // the case when it started with a decoration ("*" or "* ").
bool LastLineNeedsDecoration; bool LastLineNeedsDecoration;
// Either "* " if all lines begin with a "*", or empty. // Either "* " if all lines begin with a "*", or empty.
StringRef Decoration; StringRef Decoration;
// If this block comment has decorations, this is the column of the start of
// the decorations.
unsigned DecorationColumn;
}; };
class BreakableLineCommentSection : public BreakableComment { class BreakableLineCommentSection : public BreakableComment {
public: public:
BreakableLineCommentSection(const FormatToken &Token, unsigned StartColumn, BreakableLineCommentSection(const FormatToken &Token, unsigned StartColumn,
unsigned OriginalStartColumn, bool FirstInLine, unsigned OriginalStartColumn, bool FirstInLine,
bool InPPDirective, encoding::Encoding Encoding, bool InPPDirective, encoding::Encoding Encoding,
const FormatStyle &Style); const FormatStyle &Style);
▲ Show 20 Lines • Show All 55 Lines • Show Last 20 Lines

cfe/trunk/lib/Format/BreakableToken.cpp

Show First 20 Lines • Show All 338 Lines • ▼ Show 20 LinesBreakableBlockComment::BreakableBlockComment(
Content[0] = Lines[0]; Content[0] = Lines[0];
ContentColumn.resize(Lines.size()); ContentColumn.resize(Lines.size());
// Account for the initial '/*'. // Account for the initial '/*'.
ContentColumn[0] = StartColumn + 2; ContentColumn[0] = StartColumn + 2;
Tokens.resize(Lines.size()); Tokens.resize(Lines.size());
for (size_t i = 1; i < Lines.size(); ++i) for (size_t i = 1; i < Lines.size(); ++i)
adjustWhitespace(i, IndentDelta); adjustWhitespace(i, IndentDelta);
// Align decorations with the column of the star on the first line,
// that is one column after the start "/*".
DecorationColumn = StartColumn + 1;
// Account for comment decoration patterns like this:
//
// /*
// ** blah blah blah
// */
if (Lines.size() >= 2 && Content[1].startswith("**") &&
static_cast<unsigned>(ContentColumn[1]) == StartColumn) {
DecorationColumn = StartColumn;
}
Decoration = "* "; Decoration = "* ";
if (Lines.size() == 1 && !FirstInLine) { if (Lines.size() == 1 && !FirstInLine) {
// Comments for which FirstInLine is false can start on arbitrary column, // Comments for which FirstInLine is false can start on arbitrary column,
// and available horizontal space can be too small to align consecutive // and available horizontal space can be too small to align consecutive
// lines with the first one. // lines with the first one.
// FIXME: We could, probably, align them to current indentation level, but // FIXME: We could, probably, align them to current indentation level, but
// now we just wrap them without stars. // now we just wrap them without stars.
Decoration = ""; Decoration = "";
Show All 13 LinesBreakableBlockComment::BreakableBlockComment(
IndentAtLineBreak = ContentColumn[0] + 1; IndentAtLineBreak = ContentColumn[0] + 1;
for (size_t i = 1, e = Lines.size(); i < e; ++i) { for (size_t i = 1, e = Lines.size(); i < e; ++i) {
if (Content[i].empty()) { if (Content[i].empty()) {
if (i + 1 == e) { if (i + 1 == e) {
// Empty last line means that we already have a star as a part of the // Empty last line means that we already have a star as a part of the
// trailing */. We also need to preserve whitespace, so that */ is // trailing */. We also need to preserve whitespace, so that */ is
// correctly indented. // correctly indented.
LastLineNeedsDecoration = false; LastLineNeedsDecoration = false;
// Align the star in the last '*/' with the stars on the previous lines.
if (e >= 2 && !Decoration.empty()) {
ContentColumn[i] = DecorationColumn;
}
} else if (Decoration.empty()) { } else if (Decoration.empty()) {
// For all other lines, set the start column to 0 if they're empty, so // For all other lines, set the start column to 0 if they're empty, so
// we do not insert trailing whitespace anywhere. // we do not insert trailing whitespace anywhere.
ContentColumn[i] = 0; ContentColumn[i] = 0;
} }
continue; continue;
} }
// The first line already excludes the star. // The first line already excludes the star.
// The last line excludes the star if LastLineNeedsDecoration is false.
// For all other lines, adjust the line to exclude the star and // For all other lines, adjust the line to exclude the star and
// (optionally) the first whitespace. // (optionally) the first whitespace.
unsigned DecorationSize = Decoration.startswith(Content[i]) unsigned DecorationSize = Decoration.startswith(Content[i])
? Content[i].size() ? Content[i].size()
: Decoration.size(); : Decoration.size();
ContentColumn[i] += DecorationSize; if (DecorationSize) {
ContentColumn[i] = DecorationColumn + DecorationSize;
}
Content[i] = Content[i].substr(DecorationSize); Content[i] = Content[i].substr(DecorationSize);
if (!Decoration.startswith(Content[i])) if (!Decoration.startswith(Content[i]))
IndentAtLineBreak = IndentAtLineBreak =
std::min<int>(IndentAtLineBreak, std::max(0, ContentColumn[i])); std::min<int>(IndentAtLineBreak, std::max(0, ContentColumn[i]));
} }
IndentAtLineBreak = IndentAtLineBreak =
std::max<unsigned>(IndentAtLineBreak, Decoration.size()); std::max<unsigned>(IndentAtLineBreak, Decoration.size());
DEBUG({ DEBUG({
llvm::dbgs() << "IndentAtLineBreak " << IndentAtLineBreak << "\n"; llvm::dbgs() << "IndentAtLineBreak " << IndentAtLineBreak << "\n";
for (size_t i = 0; i < Lines.size(); ++i) { for (size_t i = 0; i < Lines.size(); ++i) {
llvm::dbgs() << i << " |" << Content[i] << "| " llvm::dbgs() << i << " |" << Content[i] << "| "
<< (Content[i].data() - Lines[i].data()) << "\n"; << "CC=" << ContentColumn[i] << "| "
<< "IN=" << (Content[i].data() - Lines[i].data()) << "\n";
} }
}); });
} }
void BreakableBlockComment::adjustWhitespace(unsigned LineIndex, void BreakableBlockComment::adjustWhitespace(unsigned LineIndex,
int IndentDelta) { int IndentDelta) {
// When in a preprocessor directive, the trailing backslash in a block comment // When in a preprocessor directive, the trailing backslash in a block comment
// is not needed, but can serve a purpose of uniformity with necessary escaped // is not needed, but can serve a purpose of uniformity with necessary escaped
▲ Show 20 Lines • Show All 473 Lines • Show Last 20 Lines

cfe/trunk/unittests/Format/FormatTest.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 1,960 Lines • ▼ Show 20 Lines
} }
TEST_F(FormatTest, EscapedNewlines) { TEST_F(FormatTest, EscapedNewlines) {
EXPECT_EQ( EXPECT_EQ(
"#define A \\\n int i; \\\n int j;", "#define A \\\n int i; \\\n int j;",
format("#define A \\\nint i;\\\n int j;", getLLVMStyleWithColumns(11))); format("#define A \\\nint i;\\\n int j;", getLLVMStyleWithColumns(11)));
EXPECT_EQ("#define A\n\nint i;", format("#define A \\\n\n int i;")); EXPECT_EQ("#define A\n\nint i;", format("#define A \\\n\n int i;"));
EXPECT_EQ("template <class T> f();", format("\\\ntemplate <class T> f();")); EXPECT_EQ("template <class T> f();", format("\\\ntemplate <class T> f();"));
EXPECT_EQ("/* \\ \\ \\\n*/", format("\\\n/* \\ \\ \\\n*/")); EXPECT_EQ("/* \\ \\ \\\n */", format("\\\n/* \\ \\ \\\n */"));
EXPECT_EQ("<a\n\\\\\n>", format("<a\n\\\\\n>")); EXPECT_EQ("<a\n\\\\\n>", format("<a\n\\\\\n>"));
} }
TEST_F(FormatTest, CalculateSpaceOnConsecutiveLinesInMacro) { TEST_F(FormatTest, CalculateSpaceOnConsecutiveLinesInMacro) {
verifyFormat("#define A \\\n" verifyFormat("#define A \\\n"
" int v( \\\n" " int v( \\\n"
" a); \\\n" " a); \\\n"
" int i;", " int i;",
▲ Show 20 Lines • Show All 7,994 Lines • Show Last 20 Lines

cfe/trunk/unittests/Format/FormatTestComments.cpp

Show First 20 Lines • Show All 931 Lines • ▼ Show 20 Lines EXPECT_EQ("/* A comment before\n"
" * a macro\n" " * a macro\n"
" * definition */\n" " * definition */\n"
"#define a b", "#define a b",
format("/* A comment before a macro definition */\n" format("/* A comment before a macro definition */\n"
"#define a b", "#define a b",
getLLVMStyleWithColumns(20))); getLLVMStyleWithColumns(20)));
EXPECT_EQ("/* some comment\n" EXPECT_EQ("/* some comment\n"
" * a comment\n" " * a comment that\n"
"* that we break\n" " * we break another\n"
" * another comment\n" " * comment we have\n"
"* we have to break\n" " * to break a left\n"
"* a left comment\n" " * comment\n"
" */", " */",
format(" /* some comment\n" format(" /* some comment\n"
" * a comment that we break\n" " * a comment that we break\n"
" * another comment we have to break\n" " * another comment we have to break\n"
"* a left comment\n" "* a left comment\n"
" */", " */",
getLLVMStyleWithColumns(20))); getLLVMStyleWithColumns(20)));
▲ Show 20 Lines • Show All 898 Lines • ▼ Show 20 LinesTEST_F(FormatTestComments, BlockComments) {
EXPECT_EQ("#define A\n" EXPECT_EQ("#define A\n"
"/* */ someCall(\n" "/* */ someCall(\n"
" parameter);", " parameter);",
format("#define A\n" format("#define A\n"
"/* */someCall(parameter);", "/* */someCall(parameter);",
getLLVMStyleWithColumns(15))); getLLVMStyleWithColumns(15)));
EXPECT_EQ("/*\n**\n*/", format("/*\n**\n*/")); EXPECT_EQ("/*\n**\n*/", format("/*\n**\n*/"));
EXPECT_EQ("/*\n" EXPECT_EQ("/*\n"
"*\n" " *\n"
" * aaaaaa\n" " * aaaaaa\n"
" * aaaaaa\n" " * aaaaaa\n"
"*/", " */",
format("/*\n" format("/*\n"
"*\n" "*\n"
" * aaaaaa aaaaaa\n" " * aaaaaa aaaaaa\n"
"*/", "*/",
getLLVMStyleWithColumns(10))); getLLVMStyleWithColumns(10)));
EXPECT_EQ("/*\n" EXPECT_EQ("/*\n"
"**\n" "**\n"
"* aaaaaa\n" "* aaaaaa\n"
▲ Show 20 Lines • Show All 288 Lines • ▼ Show 20 Lines EXPECT_EQ("int a; // line about a\n"
"// line 2 about b\n" "// line 2 about b\n"
"long b;", "long b;",
format("int a; // line about a\n" format("int a; // line about a\n"
" // line 1 about b\n" " // line 1 about b\n"
" // line 2 about b\n" " // line 2 about b\n"
" long b;", " long b;",
getLLVMStyleWithColumns(80))); getLLVMStyleWithColumns(80)));
} }
TEST_F(FormatTestComments, AlignsBlockCommentDecorations) {
EXPECT_EQ("/*\n"
" */",
format("/*\n"
"*/", getLLVMStyle()));
EXPECT_EQ("/*\n"
" */",
format("/*\n"
" */", getLLVMStyle()));
EXPECT_EQ("/*\n"
" */",
format("/*\n"
" */", getLLVMStyle()));
// Align a single line.
EXPECT_EQ("/*\n"
" * line */",
format("/*\n"
"* line */",
getLLVMStyle()));
EXPECT_EQ("/*\n"
" * line */",
format("/*\n"
" * line */",
getLLVMStyle()));
EXPECT_EQ("/*\n"
" * line */",
format("/*\n"
" * line */",
getLLVMStyle()));
EXPECT_EQ("/*\n"
" * line */",
format("/*\n"
" * line */",
getLLVMStyle()));
EXPECT_EQ("/**\n"
" * line */",
format("/**\n"
"* line */",
getLLVMStyle()));
EXPECT_EQ("/**\n"
" * line */",
format("/**\n"
" * line */",
getLLVMStyle()));
EXPECT_EQ("/**\n"
" * line */",
format("/**\n"
" * line */",
getLLVMStyle()));
EXPECT_EQ("/**\n"
" * line */",
format("/**\n"
" * line */",
getLLVMStyle()));
EXPECT_EQ("/**\n"
" * line */",
format("/**\n"
" * line */",
getLLVMStyle()));
// Align the end '*/' after a line.
EXPECT_EQ("/*\n"
" * line\n"
" */",
format("/*\n"
"* line\n"
"*/", getLLVMStyle()));
EXPECT_EQ("/*\n"
" * line\n"
" */",
format("/*\n"
" * line\n"
" */", getLLVMStyle()));
EXPECT_EQ("/*\n"
" * line\n"
" */",
format("/*\n"
" * line\n"
" */", getLLVMStyle()));
// Align two lines.
EXPECT_EQ("/* line 1\n"
" * line 2 */",
format("/* line 1\n"
" * line 2 */",
getLLVMStyle()));
EXPECT_EQ("/* line 1\n"
" * line 2 */",
format("/* line 1\n"
"* line 2 */",
getLLVMStyle()));
EXPECT_EQ("/* line 1\n"
" * line 2 */",
format("/* line 1\n"
" * line 2 */",
getLLVMStyle()));
EXPECT_EQ("/* line 1\n"
" * line 2 */",
format("/* line 1\n"
" * line 2 */",
getLLVMStyle()));
EXPECT_EQ("/* line 1\n"
" * line 2 */",
format("/* line 1\n"
" * line 2 */",
getLLVMStyle()));
EXPECT_EQ("int i; /* line 1\n"
" * line 2 */",
format("int i; /* line 1\n"
"* line 2 */",
getLLVMStyle()));
EXPECT_EQ("int i; /* line 1\n"
" * line 2 */",
format("int i; /* line 1\n"
" * line 2 */",
getLLVMStyle()));
EXPECT_EQ("int i; /* line 1\n"
" * line 2 */",
format("int i; /* line 1\n"
" * line 2 */",
getLLVMStyle()));
// Align several lines.
EXPECT_EQ("/* line 1\n"
" * line 2\n"
" * line 3 */",
format("/* line 1\n"
" * line 2\n"
"* line 3 */",
getLLVMStyle()));
EXPECT_EQ("/* line 1\n"
" * line 2\n"
" * line 3 */",
format("/* line 1\n"
" * line 2\n"
"* line 3 */",
getLLVMStyle()));
EXPECT_EQ("/*\n"
"** line 1\n"
"** line 2\n"
"*/",
format("/*\n"
"** line 1\n"
" ** line 2\n"
"*/",
getLLVMStyle()));
// Align with different indent after the decorations.
EXPECT_EQ("/*\n"
" * line 1\n"
" * line 2\n"
" * line 3\n"
" * line 4\n"
" */",
format("/*\n"
"* line 1\n"
" * line 2\n"
" * line 3\n"
"* line 4\n"
"*/", getLLVMStyle()));
// Align empty or blank lines.
EXPECT_EQ("/**\n"
" *\n"
" *\n"
" *\n"
" */",
format("/**\n"
"* \n"
" * \n"
" *\n"
"*/", getLLVMStyle()));
// Align while breaking and reflowing.
EXPECT_EQ("/*\n"
" * long long long\n"
" * long long\n"
" *\n"
" * long */",
format("/*\n"
" * long long long long\n"
" * long\n"
" *\n"
"* long */",
getLLVMStyleWithColumns(20)));
}
} // end namespace } // end namespace
} // end namespace format } // end namespace format
} // end namespace clang } // end namespace clang