r175973 - Better formatting of conditional expressions.
Daniel Jasper
djasper at google.com
Sat Feb 23 13:01:55 PST 2013
Author: djasper
Date: Sat Feb 23 15:01:55 2013
New Revision: 175973
URL: http://llvm.org/viewvc/llvm-project?rev=175973&view=rev
Log:
Better formatting of conditional expressions.
In conditional expressions, if the condition is split over multiple
lines, also break before both operands.
This prevents formattings like:
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa ==
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa ? b : c;
Which are bad, because they suggestion incorrect operator precedence:
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa ==
(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa ? b : c);
This lead to the discovery that the expression parser incorrectly
handled conditional operators and that it could also handle semicolons
(which in turn reduced the amount of special casing for for-loops). As a
side-effect, we can now apply the bin-packing configuration to the
sections of for-loops.
Modified:
cfe/trunk/lib/Format/Format.cpp
cfe/trunk/lib/Format/TokenAnnotator.cpp
cfe/trunk/unittests/Format/FormatTest.cpp
Modified: cfe/trunk/lib/Format/Format.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=175973&r1=175972&r2=175973&view=diff
==============================================================================
--- cfe/trunk/lib/Format/Format.cpp (original)
+++ cfe/trunk/lib/Format/Format.cpp Sat Feb 23 15:01:55 2013
@@ -330,8 +330,7 @@ private:
: Indent(Indent), LastSpace(LastSpace), FirstLessLess(0),
BreakBeforeClosingBrace(false), QuestionColumn(0),
AvoidBinPacking(AvoidBinPacking), BreakBeforeParameter(false),
- HasMultiParameterLine(HasMultiParameterLine), ColonPos(0),
- BreakBeforeThirdOperand(false) {}
+ HasMultiParameterLine(HasMultiParameterLine), ColonPos(0) {}
/// \brief The position to which a specific parenthesis level needs to be
/// indented.
@@ -374,9 +373,6 @@ private:
/// \brief The position of the colon in an ObjC method declaration/call.
unsigned ColonPos;
- /// \brief Break before third operand in ternary expression.
- bool BreakBeforeThirdOperand;
-
bool operator<(const ParenState &Other) const {
if (Indent != Other.Indent)
return Indent < Other.Indent;
@@ -396,8 +392,6 @@ private:
return HasMultiParameterLine;
if (ColonPos != Other.ColonPos)
return ColonPos < Other.ColonPos;
- if (BreakBeforeThirdOperand != Other.BreakBeforeThirdOperand)
- return BreakBeforeThirdOperand;
return false;
}
};
@@ -523,13 +517,11 @@ private:
}
if (Current.is(tok::question))
- State.Stack.back().BreakBeforeThirdOperand = true;
- if (Previous.is(tok::comma) && !State.Stack.back().AvoidBinPacking)
+ State.Stack.back().BreakBeforeParameter = true;
+ if ((Previous.is(tok::comma) || Previous.is(tok::semi)) &&
+ !State.Stack.back().AvoidBinPacking)
State.Stack.back().BreakBeforeParameter = false;
- if (RootToken.is(tok::kw_for))
- State.LineContainsContinuedForLoopSection = Previous.isNot(tok::semi);
-
if (!DryRun) {
unsigned NewLines =
std::max(1u, std::min(Current.FormatTok.NewlinesBefore,
@@ -546,9 +538,27 @@ private:
State.StartOfLineLevel = State.ParenLevel;
if (Current.is(tok::colon) && Current.Type != TT_ConditionalExpr)
State.Stack.back().Indent += 2;
+
+ // Any break on this level means that the parent level has been broken
+ // and we need to avoid bin packing there.
+ for (unsigned i = 0, e = State.Stack.size() - 1; i != e; ++i) {
+ State.Stack[i].BreakBeforeParameter = true;
+ }
+ // If we break after {, we should also break before the corresponding }.
+ if (Previous.is(tok::l_brace))
+ State.Stack.back().BreakBeforeClosingBrace = true;
+
+ if (State.Stack.back().AvoidBinPacking) {
+ // If we are breaking after '(', '{', '<', this is not bin packing
+ // unless AllowAllParametersOfDeclarationOnNextLine is false.
+ if ((Previous.isNot(tok::l_paren) && Previous.isNot(tok::l_brace) &&
+ Previous.Type != TT_TemplateOpener) ||
+ (!Style.AllowAllParametersOfDeclarationOnNextLine &&
+ Line.MustBeDeclaration))
+ State.Stack.back().BreakBeforeParameter = true;
+ }
} else {
- if (Current.is(tok::equal) &&
- (RootToken.is(tok::kw_for) || State.ParenLevel == 0))
+ if (Current.is(tok::equal))
State.VariablePos = State.Column - Previous.FormatTok.TokenLength;
unsigned Spaces = State.NextToken->SpacesRequiredBefore;
@@ -601,30 +611,6 @@ private:
State.Stack.back().LastSpace = State.Column;
}
- // If we break after an {, we should also break before the corresponding }.
- if (Newline && Previous.is(tok::l_brace))
- State.Stack.back().BreakBeforeClosingBrace = true;
-
- if (State.Stack.back().AvoidBinPacking && Newline &&
- (Line.First.isNot(tok::kw_for) || State.ParenLevel != 1)) {
- // If we are breaking after '(', '{', '<', this is not bin packing unless
- // AllowAllParametersOfDeclarationOnNextLine is false.
- if ((Previous.isNot(tok::l_paren) && Previous.isNot(tok::l_brace) &&
- Previous.Type != TT_TemplateOpener) ||
- (!Style.AllowAllParametersOfDeclarationOnNextLine &&
- Line.MustBeDeclaration))
- State.Stack.back().BreakBeforeParameter = true;
- }
-
- if (Newline) {
- // Any break on this level means that the parent level has been broken
- // and we need to avoid bin packing there.
- for (unsigned i = 0, e = State.Stack.size() - 1; i != e; ++i) {
- if (Line.First.isNot(tok::kw_for) || i != 1)
- State.Stack[i].BreakBeforeParameter = true;
- }
- }
-
return moveStateToNextToken(State, DryRun);
}
@@ -650,6 +636,7 @@ private:
for (unsigned i = 0, e = Current.FakeLParens; i != e; ++i) {
ParenState NewParenState = State.Stack.back();
NewParenState.Indent = std::max(State.Column, State.Stack.back().Indent);
+ NewParenState.BreakBeforeParameter = false;
State.Stack.push_back(NewParenState);
}
@@ -907,7 +894,10 @@ private:
if (State.NextToken->Parent->is(tok::semi) &&
State.LineContainsContinuedForLoopSection)
return true;
- if (State.NextToken->Parent->is(tok::comma) &&
+ if ((State.NextToken->Parent->is(tok::comma) ||
+ State.NextToken->Parent->is(tok::semi) ||
+ State.NextToken->is(tok::question) ||
+ State.NextToken->Type == TT_ConditionalExpr) &&
State.Stack.back().BreakBeforeParameter &&
!isTrailingComment(*State.NextToken) &&
State.NextToken->isNot(tok::r_paren) &&
@@ -923,9 +913,6 @@ private:
(State.NextToken->Parent->ClosesTemplateDeclaration &&
State.ParenLevel == 0)))
return true;
- if (State.NextToken->is(tok::colon) &&
- State.Stack.back().BreakBeforeThirdOperand)
- return true;
return false;
}
Modified: cfe/trunk/lib/Format/TokenAnnotator.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/TokenAnnotator.cpp?rev=175973&r1=175972&r2=175973&view=diff
==============================================================================
--- cfe/trunk/lib/Format/TokenAnnotator.cpp (original)
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp Sat Feb 23 15:01:55 2013
@@ -724,7 +724,7 @@ public:
ExpressionParser(AnnotatedLine &Line) : Current(&Line.First) {}
/// \brief Parse expressions with the given operatore precedence.
- void parse(prec::Level Precedence = prec::Unknown) {
+ void parse(int Precedence = 0) {
if (Precedence > prec::PointerToMember || Current == NULL)
return;
@@ -740,18 +740,27 @@ public:
AnnotatedToken *Start = Current;
bool OperatorFound = false;
- while (Current != NULL) {
+ while (Current) {
// Consume operators with higher precedence.
parse(prec::Level(Precedence + 1));
+ int CurrentPrecedence = 0;
+ if (Current) {
+ if (Current->Type == TT_ConditionalExpr)
+ CurrentPrecedence = 1 + (int) prec::Conditional;
+ else if (Current->is(tok::semi))
+ CurrentPrecedence = 1;
+ else if (Current->Type == TT_BinaryOperator || Current->is(tok::comma))
+ CurrentPrecedence = 1 + (int) getPrecedence(*Current);
+ }
+
// At the end of the line or when an operator with higher precedence is
// found, insert fake parenthesis and return.
- if (Current == NULL || Current->is(tok::semi) || closesScope(*Current) ||
- ((Current->Type == TT_BinaryOperator || Current->is(tok::comma)) &&
- getPrecedence(*Current) < Precedence)) {
+ if (Current == NULL || closesScope(*Current) ||
+ (CurrentPrecedence != 0 && CurrentPrecedence < Precedence)) {
if (OperatorFound) {
++Start->FakeLParens;
- if (Current != NULL)
+ if (Current)
++Current->Parent->FakeRParens;
}
return;
@@ -759,14 +768,21 @@ public:
// Consume scopes: (), [], <> and {}
if (opensScope(*Current)) {
- while (Current != NULL && !closesScope(*Current)) {
+ AnnotatedToken *Left = Current;
+ while (Current && !closesScope(*Current)) {
next();
parse();
}
+ // Remove fake parens that just duplicate the real parens.
+ if (Current && Left->Children[0].FakeLParens > 0 &&
+ Current->Parent->FakeRParens > 0) {
+ --Left->Children[0].FakeLParens;
+ --Current->Parent->FakeRParens;
+ }
next();
} else {
// Operator found.
- if (getPrecedence(*Current) == Precedence)
+ if (CurrentPrecedence == Precedence)
OperatorFound = true;
next();
Modified: cfe/trunk/unittests/Format/FormatTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=175973&r1=175972&r2=175973&view=diff
==============================================================================
--- cfe/trunk/unittests/Format/FormatTest.cpp (original)
+++ cfe/trunk/unittests/Format/FormatTest.cpp Sat Feb 23 15:01:55 2013
@@ -306,6 +306,11 @@ TEST_F(FormatTest, FormatsForLoop) {
" aaaaaaaaaaaaaaaa);\n"
" aaaaaaaaaaa++, bbbbbbbbbbbbbbbbb++) {\n"
"}");
+ verifyGoogleFormat(
+ "for (std::vector<UnwrappedLine>::iterator I = UnwrappedLines.begin(),\n"
+ " E = UnwrappedLines.end();\n"
+ " I != E;\n"
+ " ++I) {\n}");
}
TEST_F(FormatTest, RangeBasedForLoops) {
@@ -1397,9 +1402,14 @@ TEST_F(FormatTest, BreaksConditionalExpr
verifyFormat("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
" ? aaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
" : aaaaaaaaaaaaaaaaaaaaaaaaaaa;");
-
- // FIXME: The trailing third parameter here is kind of hidden. Prefer putting
- // it on the next line.
+ verifyFormat(
+ "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa == aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
+ " ? aaaaaaaaaaaaaaa\n"
+ " : aaaaaaaaaaaaaaa;");
+ verifyFormat("f(aaaaaaaaaaaaaaaa == // force break\n"
+ " aaaaaaaaa\n"
+ " ? b\n"
+ " : c);");
verifyFormat(
"unsigned Indent =\n"
" format(TheLine.First, IndentForLevel[TheLine.Level] >= 0\n"
@@ -1408,6 +1418,14 @@ TEST_F(FormatTest, BreaksConditionalExpr
" TheLine.InPPDirective, PreviousEndOfLineColumn);",
getLLVMStyleWithColumns(70));
+ verifyGoogleFormat(
+ "void f() {\n"
+ " g(aaa,\n"
+ " aaaaaaaaaa == aaaaaaaaaa ? aaaa : aaaaa,\n"
+ " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa == aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
+ " ? aaaaaaaaaaaaaaa\n"
+ " : aaaaaaaaaaaaaaa);\n"
+ "}");
}
TEST_F(FormatTest, DeclarationsOfMultipleVariables) {
More information about the cfe-commits
mailing list