Hi klimek,
A trailing block comment having multiple lines would cause extremely
high penalties if the summary length of its lines is more than the column limit.
Fixed by always considering only the last line of a multi-line block comment.
Removed a long-standing FIXME from relevant tests and added a motivating test
modelled after problem cases from real code.
http://llvm-reviews.chandlerc.com/D1010
Files:
lib/Format/Format.cpp
unittests/Format/FormatTest.cpp
Index: lib/Format/Format.cpp
===================================================================
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -832,17 +832,17 @@
}
if (Current.UnbreakableTailLength >= getColumnLimit())
return 0;
- unsigned RemainingSpace = getColumnLimit() - Current.UnbreakableTailLength;
+ unsigned RemainingSpace = getColumnLimit() - Current.UnbreakableTailLength;
bool BreakInserted = false;
unsigned Penalty = 0;
- unsigned PositionAfterLastLineInToken = 0;
+ unsigned RemainingTokenColumns = 0;
for (unsigned LineIndex = 0, EndIndex = Token->getLineCount();
LineIndex != EndIndex; ++LineIndex) {
if (!DryRun)
Token->replaceWhitespaceBefore(LineIndex, Whitespaces);
unsigned TailOffset = 0;
- unsigned RemainingTokenColumns = Token->getLineLengthAfterSplit(
+ RemainingTokenColumns = Token->getLineLengthAfterSplit(
LineIndex, TailOffset, StringRef::npos);
while (RemainingTokenColumns > RemainingSpace) {
BreakableToken::Split Split =
@@ -868,11 +868,11 @@
RemainingTokenColumns = NewRemainingTokenColumns;
BreakInserted = true;
}
- PositionAfterLastLineInToken = RemainingTokenColumns;
}
+ State.Column = RemainingTokenColumns;
+
if (BreakInserted) {
- State.Column = PositionAfterLastLineInToken;
// If we break the token inside a parameter list, we need to break before
// the next parameter on all levels, so that the next parameter is
clearly
// visible. Line comments already introduce a break.
Index: unittests/Format/FormatTest.cpp
===================================================================
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -755,7 +755,7 @@
getLLVMStyleWithColumns(33)));
}
-TEST_F(FormatTest, UnderstandsMultiLineComments) {
+TEST_F(FormatTest, UnderstandsBlockComments) {
verifyFormat("f(/*test=*/ true);");
EXPECT_EQ(
"f(aaaaaaaaaaaaaaaaaaaaaaaaa, /* Trailing comment for aa... */\n"
@@ -786,7 +786,7 @@
NoBinPacking);
}
-TEST_F(FormatTest, AlignsMultiLineComments) {
+TEST_F(FormatTest, AlignsBlockComments) {
EXPECT_EQ("/*\n"
" * Really multi-line\n"
" * comment.\n"
@@ -836,6 +836,13 @@
" * line. */"));
}
+TEST_F(FormatTest, CorrectlyHandlesLengthOfBlockComments) {
+ EXPECT_EQ("double *x; /* aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
+ " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa */",
+ format("double *x; /* aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
+ " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
*/"));
+}
+
TEST_F(FormatTest, SplitsLongCxxComments) {
EXPECT_EQ("// A comment that\n"
"// doesn't fit on\n"
@@ -919,24 +926,19 @@
}
TEST_F(FormatTest, MultiLineCommentsInDefines) {
- // FIXME: The line breaks are still suboptimal (current guess
- // is that this is due to the token length being misused), but
- // the comment handling is correct.
- EXPECT_EQ("#define A( \\\n"
- " x) /* \\\n"
- "a comment \\\n"
- "inside */ \\\n"
+ EXPECT_EQ("#define A(x) /* \\\n"
+ " a comment \\\n"
+ " inside */ \\\n"
" f();",
format("#define A(x) /* \\\n"
" a comment \\\n"
" inside */ \\\n"
" f();",
getLLVMStyleWithColumns(17)));
- EXPECT_EQ("#define A(x) /* \\\n"
- " a \\\n"
- " comment \\\n"
- " inside \\\n"
- " */ \\\n"
+ EXPECT_EQ("#define A( \\\n"
+ " x) /* \\\n"
+ " a comment \\\n"
+ " inside */ \\\n"
" f();",
format("#define A( \\\n"
" x) /* \\\n"
Index: lib/Format/Format.cpp
===================================================================
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -832,17 +832,17 @@
}
if (Current.UnbreakableTailLength >= getColumnLimit())
return 0;
- unsigned RemainingSpace = getColumnLimit() - Current.UnbreakableTailLength;
+ unsigned RemainingSpace = getColumnLimit() - Current.UnbreakableTailLength;
bool BreakInserted = false;
unsigned Penalty = 0;
- unsigned PositionAfterLastLineInToken = 0;
+ unsigned RemainingTokenColumns = 0;
for (unsigned LineIndex = 0, EndIndex = Token->getLineCount();
LineIndex != EndIndex; ++LineIndex) {
if (!DryRun)
Token->replaceWhitespaceBefore(LineIndex, Whitespaces);
unsigned TailOffset = 0;
- unsigned RemainingTokenColumns = Token->getLineLengthAfterSplit(
+ RemainingTokenColumns = Token->getLineLengthAfterSplit(
LineIndex, TailOffset, StringRef::npos);
while (RemainingTokenColumns > RemainingSpace) {
BreakableToken::Split Split =
@@ -868,11 +868,11 @@
RemainingTokenColumns = NewRemainingTokenColumns;
BreakInserted = true;
}
- PositionAfterLastLineInToken = RemainingTokenColumns;
}
+ State.Column = RemainingTokenColumns;
+
if (BreakInserted) {
- State.Column = PositionAfterLastLineInToken;
// If we break the token inside a parameter list, we need to break before
// the next parameter on all levels, so that the next parameter is clearly
// visible. Line comments already introduce a break.
Index: unittests/Format/FormatTest.cpp
===================================================================
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -755,7 +755,7 @@
getLLVMStyleWithColumns(33)));
}
-TEST_F(FormatTest, UnderstandsMultiLineComments) {
+TEST_F(FormatTest, UnderstandsBlockComments) {
verifyFormat("f(/*test=*/ true);");
EXPECT_EQ(
"f(aaaaaaaaaaaaaaaaaaaaaaaaa, /* Trailing comment for aa... */\n"
@@ -786,7 +786,7 @@
NoBinPacking);
}
-TEST_F(FormatTest, AlignsMultiLineComments) {
+TEST_F(FormatTest, AlignsBlockComments) {
EXPECT_EQ("/*\n"
" * Really multi-line\n"
" * comment.\n"
@@ -836,6 +836,13 @@
" * line. */"));
}
+TEST_F(FormatTest, CorrectlyHandlesLengthOfBlockComments) {
+ EXPECT_EQ("double *x; /* aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
+ " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa */",
+ format("double *x; /* aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
+ " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa */"));
+}
+
TEST_F(FormatTest, SplitsLongCxxComments) {
EXPECT_EQ("// A comment that\n"
"// doesn't fit on\n"
@@ -919,24 +926,19 @@
}
TEST_F(FormatTest, MultiLineCommentsInDefines) {
- // FIXME: The line breaks are still suboptimal (current guess
- // is that this is due to the token length being misused), but
- // the comment handling is correct.
- EXPECT_EQ("#define A( \\\n"
- " x) /* \\\n"
- "a comment \\\n"
- "inside */ \\\n"
+ EXPECT_EQ("#define A(x) /* \\\n"
+ " a comment \\\n"
+ " inside */ \\\n"
" f();",
format("#define A(x) /* \\\n"
" a comment \\\n"
" inside */ \\\n"
" f();",
getLLVMStyleWithColumns(17)));
- EXPECT_EQ("#define A(x) /* \\\n"
- " a \\\n"
- " comment \\\n"
- " inside \\\n"
- " */ \\\n"
+ EXPECT_EQ("#define A( \\\n"
+ " x) /* \\\n"
+ " a comment \\\n"
+ " inside */ \\\n"
" f();",
format("#define A( \\\n"
" x) /* \\\n"
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
