r315893 - [clang-format] Break non-trailing comments, try 2
Krasimir Georgiev via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 16 02:08:53 PDT 2017
Author: krasimir
Date: Mon Oct 16 02:08:53 2017
New Revision: 315893
URL: http://llvm.org/viewvc/llvm-project?rev=315893&view=rev
Log:
[clang-format] Break non-trailing comments, try 2
Summary:
This patch enables `BreakableToken` to manage the formatting of non-trailing
block comments. It is a refinement of https://reviews.llvm.org/D37007.
We discovered that the optimizer outsmarts us on cases where breaking the comment
costs considerably less than breaking after the comment. This patch addresses
this by ensuring that a newline is inserted between a block comment and the next
token.
Reviewers: djasper
Reviewed By: djasper
Subscribers: cfe-commits, klimek
Differential Revision: https://reviews.llvm.org/D37695
Modified:
cfe/trunk/lib/Format/BreakableToken.cpp
cfe/trunk/lib/Format/BreakableToken.h
cfe/trunk/lib/Format/ContinuationIndenter.cpp
cfe/trunk/lib/Format/ContinuationIndenter.h
cfe/trunk/unittests/Format/FormatTestComments.cpp
cfe/trunk/unittests/Format/FormatTestJS.cpp
Modified: cfe/trunk/lib/Format/BreakableToken.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/BreakableToken.cpp?rev=315893&r1=315892&r2=315893&view=diff
==============================================================================
--- cfe/trunk/lib/Format/BreakableToken.cpp (original)
+++ cfe/trunk/lib/Format/BreakableToken.cpp Mon Oct 16 02:08:53 2017
@@ -599,6 +599,12 @@ unsigned BreakableBlockComment::getLineL
}
}
+bool BreakableBlockComment::introducesBreakBefore(unsigned LineIndex) const {
+ // A break is introduced when we want delimiters on newline.
+ return LineIndex == 0 && DelimitersOnNewline &&
+ Lines[0].substr(1).find_first_not_of(Blanks) != StringRef::npos;
+}
+
void BreakableBlockComment::replaceWhitespaceBefore(
unsigned LineIndex, unsigned PreviousEndColumn, unsigned ColumnLimit,
Split SplitBefore, WhitespaceManager &Whitespaces) {
Modified: cfe/trunk/lib/Format/BreakableToken.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/BreakableToken.h?rev=315893&r1=315892&r2=315893&view=diff
==============================================================================
--- cfe/trunk/lib/Format/BreakableToken.h (original)
+++ cfe/trunk/lib/Format/BreakableToken.h Mon Oct 16 02:08:53 2017
@@ -58,6 +58,8 @@ struct FormatStyle;
/// operations that might be executed before the main line breaking occurs:
/// - getSplitBefore, for finding a split such that the content preceding it
/// needs to be specially reflown,
+/// - introducesBreakBefore, for checking if reformatting the beginning
+/// of the content introduces a line break before it,
/// - getLineLengthAfterSplitBefore, for calculating the line length in columns
/// of the remainder of the content after the beginning of the content has
/// been reformatted, and
@@ -135,6 +137,12 @@ public:
return Split(StringRef::npos, 0);
}
+ /// \brief Returns if a break before the content at \p LineIndex will be
+ /// inserted after the whitespace preceding the content has been reformatted.
+ virtual bool introducesBreakBefore(unsigned LineIndex) const {
+ return false;
+ }
+
/// \brief Returns the number of columns required to format the piece of line
/// at \p LineIndex after the content preceding the whitespace range specified
/// \p SplitBefore has been reformatted, but before any breaks are made to
@@ -339,6 +347,7 @@ public:
Split getSplitBefore(unsigned LineIndex, unsigned PreviousEndColumn,
unsigned ColumnLimit,
llvm::Regex &CommentPragmasRegex) const override;
+ bool introducesBreakBefore(unsigned LineIndex) const override;
unsigned getLineLengthAfterSplitBefore(unsigned LineIndex,
unsigned TailOffset,
unsigned PreviousEndColumn,
Modified: cfe/trunk/lib/Format/ContinuationIndenter.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/ContinuationIndenter.cpp?rev=315893&r1=315892&r2=315893&view=diff
==============================================================================
--- cfe/trunk/lib/Format/ContinuationIndenter.cpp (original)
+++ cfe/trunk/lib/Format/ContinuationIndenter.cpp Mon Oct 16 02:08:53 2017
@@ -106,6 +106,7 @@ LineState ContinuationIndenter::getIniti
/*AvoidBinPacking=*/false,
/*NoLineBreak=*/false));
State.LineContainsContinuedForLoopSection = false;
+ State.NoContinuation = false;
State.StartOfStringLiteral = 0;
State.StartOfLineLevel = 0;
State.LowestLevelOnLine = 0;
@@ -322,6 +323,12 @@ bool ContinuationIndenter::mustBreak(con
Previous.TokenText == "\'\\n\'"))))
return true;
+ if (Previous.is(TT_BlockComment) && Previous.IsMultiline)
+ return true;
+
+ if (State.NoContinuation)
+ return true;
+
return false;
}
@@ -331,6 +338,8 @@ unsigned ContinuationIndenter::addTokenT
const FormatToken &Current = *State.NextToken;
assert(!State.Stack.empty());
+ State.NoContinuation = false;
+
if ((Current.is(TT_ImplicitStringLiteral) &&
(Current.Previous->Tok.getIdentifierInfo() == nullptr ||
Current.Previous->Tok.getIdentifierInfo()->getPPKeywordID() ==
@@ -1286,7 +1295,7 @@ unsigned ContinuationIndenter::breakProt
return 0;
}
} else if (Current.is(TT_BlockComment)) {
- if (!Current.isTrailingComment() || !Style.ReflowComments ||
+ if (!Style.ReflowComments ||
// If a comment token switches formatting, like
// /* clang-format on */, we don't want to break it further,
// but we may still want to adjust its indentation.
@@ -1332,6 +1341,7 @@ unsigned ContinuationIndenter::breakProt
ReflowInProgress = SplitBefore.first != StringRef::npos;
TailOffset =
ReflowInProgress ? (SplitBefore.first + SplitBefore.second) : 0;
+ BreakInserted = BreakInserted || Token->introducesBreakBefore(LineIndex);
if (!DryRun)
Token->replaceWhitespaceBefore(LineIndex, RemainingTokenColumns,
RemainingSpace, SplitBefore, Whitespaces);
@@ -1408,6 +1418,9 @@ unsigned ContinuationIndenter::breakProt
State.Stack[i].BreakBeforeParameter = true;
}
+ if (Current.is(TT_BlockComment))
+ State.NoContinuation = true;
+
Penalty += Current.isStringLiteral() ? Style.PenaltyBreakString
: Style.PenaltyBreakComment;
Modified: cfe/trunk/lib/Format/ContinuationIndenter.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/ContinuationIndenter.h?rev=315893&r1=315892&r2=315893&view=diff
==============================================================================
--- cfe/trunk/lib/Format/ContinuationIndenter.h (original)
+++ cfe/trunk/lib/Format/ContinuationIndenter.h Mon Oct 16 02:08:53 2017
@@ -318,6 +318,9 @@ struct LineState {
/// \brief \c true if this line contains a continued for-loop section.
bool LineContainsContinuedForLoopSection;
+ /// \brief \c true if \p NextToken should not continue this line.
+ bool NoContinuation;
+
/// \brief The \c NestingLevel at the start of this line.
unsigned StartOfLineLevel;
@@ -364,6 +367,8 @@ struct LineState {
if (LineContainsContinuedForLoopSection !=
Other.LineContainsContinuedForLoopSection)
return LineContainsContinuedForLoopSection;
+ if (NoContinuation != Other.NoContinuation)
+ return NoContinuation;
if (StartOfLineLevel != Other.StartOfLineLevel)
return StartOfLineLevel < Other.StartOfLineLevel;
if (LowestLevelOnLine != Other.LowestLevelOnLine)
Modified: cfe/trunk/unittests/Format/FormatTestComments.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTestComments.cpp?rev=315893&r1=315892&r2=315893&view=diff
==============================================================================
--- cfe/trunk/unittests/Format/FormatTestComments.cpp (original)
+++ cfe/trunk/unittests/Format/FormatTestComments.cpp Mon Oct 16 02:08:53 2017
@@ -2407,6 +2407,57 @@ TEST_F(FormatTestComments, BlockComments
getLLVMStyleWithColumns(15)));
}
+TEST_F(FormatTestComments, BreaksAfterMultilineBlockCommentsInParamLists) {
+ EXPECT_EQ("a = f(/* long\n"
+ " long\n"
+ " */\n"
+ " a);",
+ format("a = f(/* long long */ a);", getLLVMStyleWithColumns(15)));
+
+ EXPECT_EQ("a = f(/* long\n"
+ " long\n"
+ " */\n"
+ " a);",
+ format("a = f(/* long\n"
+ " long\n"
+ " */a);",
+ getLLVMStyleWithColumns(15)));
+
+ EXPECT_EQ("a = f(/* long\n"
+ " long\n"
+ " */\n"
+ " a);",
+ format("a = f(/* long\n"
+ " long\n"
+ " */ a);",
+ getLLVMStyleWithColumns(15)));
+
+ EXPECT_EQ("a = f(/* long\n"
+ " long\n"
+ " */\n"
+ " (1 + 1));",
+ format("a = f(/* long\n"
+ " long\n"
+ " */ (1 + 1));",
+ getLLVMStyleWithColumns(15)));
+
+ EXPECT_EQ(
+ "a = f(a,\n"
+ " /* long\n"
+ " long\n"
+ " */\n"
+ " b);",
+ format("a = f(a, /* long long */ b);", getLLVMStyleWithColumns(15)));
+
+ EXPECT_EQ(
+ "a = f(a,\n"
+ " /* long\n"
+ " long\n"
+ " */\n"
+ " (1 + 1));",
+ format("a = f(a, /* long long */ (1 + 1));", getLLVMStyleWithColumns(15)));
+}
+
TEST_F(FormatTestComments, IndentLineCommentsInStartOfBlockAtEndOfFile) {
verifyFormat("{\n"
" // a\n"
@@ -2805,6 +2856,22 @@ TEST_F(FormatTestComments, NoCrush_Bug34
getLLVMStyleWithColumns(80)));
// clang-format on
}
+
+TEST_F(FormatTestComments, NonTrailingBlockComments) {
+ verifyFormat("const /** comment comment */ A = B;",
+ getLLVMStyleWithColumns(40));
+
+ verifyFormat("const /** comment comment comment */ A =\n"
+ " B;",
+ getLLVMStyleWithColumns(40));
+
+ EXPECT_EQ("const /** comment comment comment\n"
+ " comment */\n"
+ " A = B;",
+ format("const /** comment comment comment comment */\n"
+ " A = B;",
+ getLLVMStyleWithColumns(40)));
+}
} // end namespace
} // end namespace format
} // end namespace clang
Modified: cfe/trunk/unittests/Format/FormatTestJS.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTestJS.cpp?rev=315893&r1=315892&r2=315893&view=diff
==============================================================================
--- cfe/trunk/unittests/Format/FormatTestJS.cpp (original)
+++ cfe/trunk/unittests/Format/FormatTestJS.cpp Mon Oct 16 02:08:53 2017
@@ -65,6 +65,27 @@ protected:
TEST_F(FormatTestJS, BlockComments) {
verifyFormat("/* aaaaaaaaaaaaa */ aaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
" aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);");
+ // Breaks after a single line block comment.
+ EXPECT_EQ("aaaaa = bbbb.ccccccccccccccc(\n"
+ " /** @type_{!cccc.rrrrrrr.MMMMMMMMMMMM.LLLLLLLLLLL.lala} */\n"
+ " mediaMessage);",
+ format("aaaaa = bbbb.ccccccccccccccc(\n"
+ " /** "
+ "@type_{!cccc.rrrrrrr.MMMMMMMMMMMM.LLLLLLLLLLL.lala} */ "
+ "mediaMessage);",
+ getGoogleJSStyleWithColumns(70)));
+ // Breaks after a multiline block comment.
+ EXPECT_EQ(
+ "aaaaa = bbbb.ccccccccccccccc(\n"
+ " /**\n"
+ " * @type_{!cccc.rrrrrrr.MMMMMMMMMMMM.LLLLLLLLLLL.lala}\n"
+ " */\n"
+ " mediaMessage);",
+ format("aaaaa = bbbb.ccccccccccccccc(\n"
+ " /**\n"
+ " * @type_{!cccc.rrrrrrr.MMMMMMMMMMMM.LLLLLLLLLLL.lala}\n"
+ " */ mediaMessage);",
+ getGoogleJSStyleWithColumns(70)));
}
TEST_F(FormatTestJS, JSDocComments) {
More information about the cfe-commits
mailing list