From 11a0ac66e10658f9eec87db0e846f6f5eaafc9cf Mon Sep 17 00:00:00 2001 From: Daniel Jasper Date: Fri, 12 Dec 2014 09:40:58 +0000 Subject: [PATCH] clang-format: Revamp nested block formatting. This fixed llvm.org/PR21804 and hopefully a few other strange cases. Before: if (blah_blah(whatever, whatever, [] { doo_dah(); doo_dah(); })) { } } After: if (blah_blah(whatever, whatever, [] { doo_dah(); doo_dah(); })) { } } llvm-svn: 224112 --- clang/lib/Format/ContinuationIndenter.cpp | 104 ++++++++-------------------- clang/lib/Format/ContinuationIndenter.h | 9 ++- clang/lib/Format/FormatToken.h | 5 +- clang/lib/Format/UnwrappedLineFormatter.cpp | 19 +++-- clang/unittests/Format/FormatTest.cpp | 9 ++- clang/unittests/Format/FormatTestJS.cpp | 32 +++++++-- 6 files changed, 88 insertions(+), 90 deletions(-) diff --git a/clang/lib/Format/ContinuationIndenter.cpp b/clang/lib/Format/ContinuationIndenter.cpp index 8a57b09..e709cfb 100644 --- a/clang/lib/Format/ContinuationIndenter.cpp +++ b/clang/lib/Format/ContinuationIndenter.cpp @@ -324,25 +324,27 @@ void ContinuationIndenter::addTokenOnCurrentLine(LineState &State, bool DryRun, State.Column += Spaces; if (Current.isNot(tok::comment) && Previous.is(tok::l_paren) && - Previous.Previous && Previous.Previous->isOneOf(tok::kw_if, tok::kw_for)) + Previous.Previous && + Previous.Previous->isOneOf(tok::kw_if, tok::kw_for)) { // Treat the condition inside an if as if it was a second function // parameter, i.e. let nested calls have a continuation indent. State.Stack.back().LastSpace = State.Column; - else if (!Current.isOneOf(tok::comment, tok::caret) && - (Previous.is(tok::comma) || - (Previous.is(tok::colon) && Previous.is(TT_ObjCMethodExpr)))) + State.Stack.back().NestedBlockIndent = State.Column; + } else if (!Current.isOneOf(tok::comment, tok::caret) && + (Previous.is(tok::comma) || + (Previous.is(tok::colon) && Previous.is(TT_ObjCMethodExpr)))) { State.Stack.back().LastSpace = State.Column; - else if ((Previous.isOneOf(TT_BinaryOperator, TT_ConditionalExpr, - TT_CtorInitializerColon)) && - ((Previous.getPrecedence() != prec::Assignment && - (Previous.isNot(tok::lessless) || Previous.OperatorIndex != 0 || - !Previous.LastOperator)) || - Current.StartsBinaryExpression)) + } else if ((Previous.isOneOf(TT_BinaryOperator, TT_ConditionalExpr, + TT_CtorInitializerColon)) && + ((Previous.getPrecedence() != prec::Assignment && + (Previous.isNot(tok::lessless) || Previous.OperatorIndex != 0 || + !Previous.LastOperator)) || + Current.StartsBinaryExpression)) { // Always indent relative to the RHS of the expression unless this is a // simple assignment without binary expression on the RHS. Also indent // relative to unary operators and the colons of constructor initializers. State.Stack.back().LastSpace = State.Column; - else if (Previous.is(TT_InheritanceColon)) { + } else if (Previous.is(TT_InheritanceColon)) { State.Stack.back().Indent = State.Column; State.Stack.back().LastSpace = State.Column; } else if (Previous.opensScope()) { @@ -393,6 +395,7 @@ unsigned ContinuationIndenter::addTokenOnNewLine(LineState &State, Penalty += Style.PenaltyBreakFirstLessLess; State.Column = getNewLineColumn(State); + State.Stack.back().NestedBlockIndent = State.Column; if (NextNonComment->isMemberAccess()) { if (State.Stack.back().CallContinuation == 0) State.Stack.back().CallContinuation = State.Column; @@ -513,12 +516,10 @@ unsigned ContinuationIndenter::getNewLineColumn(const LineState &State) { return Current.NestingLevel == 0 ? State.FirstIndent : State.Stack.back().Indent; if (Current.isOneOf(tok::r_brace, tok::r_square)) { - if (State.Stack.size() > 1 && - State.Stack[State.Stack.size() - 2].NestedBlockInlined) - return State.FirstIndent; - if (Current.closesBlockTypeList(Style) || - (Current.MatchingParen && - Current.MatchingParen->BlockKind == BK_BracedInit)) + if (Current.closesBlockTypeList(Style)) + return State.Stack[State.Stack.size() - 2].NestedBlockIndent; + if (Current.MatchingParen && + Current.MatchingParen->BlockKind == BK_BracedInit) return State.Stack[State.Stack.size() - 2].LastSpace; return State.FirstIndent; } @@ -664,7 +665,7 @@ unsigned ContinuationIndenter::moveStateToNextToken(LineState &State, !Previous->isOneOf(TT_DictLiteral, TT_ObjCMethodExpr)) { State.Stack.back().NestedBlockInlined = !Newline && - (Previous->isNot(tok::l_paren) || Previous->ParameterCount > 1) && !Newline; + (Previous->isNot(tok::l_paren) || Previous->ParameterCount > 1); } moveStatePastFakeLParens(State, Newline); @@ -777,9 +778,8 @@ void ContinuationIndenter::moveStatePastFakeLParens(LineState &State, } } -// Remove the fake r_parens after 'Tok'. -static void consumeRParens(LineState& State, const FormatToken &Tok) { - for (unsigned i = 0, e = Tok.FakeRParens; i != e; ++i) { +void ContinuationIndenter::moveStatePastFakeRParens(LineState &State) { + for (unsigned i = 0, e = State.NextToken->FakeRParens; i != e; ++i) { unsigned VariablePos = State.Stack.back().VariablePos; assert(State.Stack.size() > 1); if (State.Stack.size() == 1) { @@ -791,45 +791,6 @@ static void consumeRParens(LineState& State, const FormatToken &Tok) { } } -// Returns whether 'Tok' opens or closes a scope requiring special handling -// of the subsequent fake r_parens. -// -// For example, if this is an l_brace starting a nested block, we pretend (wrt. -// to indentation) that we already consumed the corresponding r_brace. Thus, we -// remove all ParenStates caused by fake parentheses that end at the r_brace. -// The net effect of this is that we don't indent relative to the l_brace, if -// the nested block is the last parameter of a function. This formats: -// -// SomeFunction(a, [] { -// f(); // break -// }); -// -// instead of: -// SomeFunction(a, [] { -// f(); // break -// }); -static bool fakeRParenSpecialCase(const LineState &State) { - const FormatToken &Tok = *State.NextToken; - if (!Tok.MatchingParen) - return false; - const FormatToken *Left = &Tok; - if (Tok.isOneOf(tok::r_brace, tok::r_square)) - Left = Tok.MatchingParen; - return !State.Stack.back().HasMultipleNestedBlocks && - Left->isOneOf(tok::l_brace, tok::l_square) && - (Left->BlockKind == BK_Block || - Left->isOneOf(TT_ArrayInitializerLSquare, TT_DictLiteral)); -} - -void ContinuationIndenter::moveStatePastFakeRParens(LineState &State) { - // Don't remove FakeRParens attached to r_braces that surround nested blocks - // as they will have been removed early (see above). - if (fakeRParenSpecialCase(State)) - return; - - consumeRParens(State, *State.NextToken); -} - void ContinuationIndenter::moveStatePastScopeOpener(LineState &State, bool Newline) { const FormatToken &Current = *State.NextToken; @@ -846,16 +807,12 @@ void ContinuationIndenter::moveStatePastScopeOpener(LineState &State, bool AvoidBinPacking; bool BreakBeforeParameter = false; if (Current.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare)) { - if (fakeRParenSpecialCase(State)) - consumeRParens(State, *Current.MatchingParen); - - NewIndent = State.Stack.back().LastSpace; if (Current.opensBlockTypeList(Style)) { - NewIndent += Style.IndentWidth; + NewIndent = State.Stack.back().NestedBlockIndent + Style.IndentWidth; NewIndent = std::min(State.Column + 2, NewIndent); ++NewIndentLevel; } else { - NewIndent += Style.ContinuationIndentWidth; + NewIndent = State.Stack.back().LastSpace + Style.ContinuationIndentWidth; NewIndent = std::min(State.Column + 1, NewIndent); } const FormatToken *NextNoComment = Current.getNextNonComment(); @@ -884,9 +841,11 @@ void ContinuationIndenter::moveStatePastScopeOpener(LineState &State, bool NoLineBreak = State.Stack.back().NoLineBreak || (Current.is(TT_TemplateOpener) && State.Stack.back().ContainsUnwrappedBuilder); + unsigned NestedBlockIndent = State.Stack.back().NestedBlockIndent; State.Stack.push_back(ParenState(NewIndent, NewIndentLevel, State.Stack.back().LastSpace, AvoidBinPacking, NoLineBreak)); + State.Stack.back().NestedBlockIndent = NestedBlockIndent; State.Stack.back().BreakBeforeParameter = BreakBeforeParameter; State.Stack.back().HasMultipleNestedBlocks = Current.BlockParameterCount > 1; } @@ -913,20 +872,17 @@ void ContinuationIndenter::moveStatePastScopeCloser(LineState &State) { } void ContinuationIndenter::moveStateToNewBlock(LineState &State) { - // If we have already found more than one lambda introducers on this level, we - // opt out of this because similarity between the lambdas is more important. - if (fakeRParenSpecialCase(State)) - consumeRParens(State, *State.NextToken->MatchingParen); - + unsigned NestedBlockIndent = State.Stack.back().NestedBlockIndent; // ObjC block sometimes follow special indentation rules. unsigned NewIndent = - State.Stack.back().LastSpace + (State.NextToken->is(TT_ObjCBlockLBrace) - ? Style.ObjCBlockIndentWidth - : Style.IndentWidth); + NestedBlockIndent + (State.NextToken->is(TT_ObjCBlockLBrace) + ? Style.ObjCBlockIndentWidth + : Style.IndentWidth); State.Stack.push_back(ParenState( NewIndent, /*NewIndentLevel=*/State.Stack.back().IndentLevel + 1, State.Stack.back().LastSpace, /*AvoidBinPacking=*/true, State.Stack.back().NoLineBreak)); + State.Stack.back().NestedBlockIndent = NestedBlockIndent; State.Stack.back().BreakBeforeParameter = true; } diff --git a/clang/lib/Format/ContinuationIndenter.h b/clang/lib/Format/ContinuationIndenter.h index 004ddeb..36691d9 100644 --- a/clang/lib/Format/ContinuationIndenter.h +++ b/clang/lib/Format/ContinuationIndenter.h @@ -148,7 +148,8 @@ struct ParenState { ParenState(unsigned Indent, unsigned IndentLevel, unsigned LastSpace, bool AvoidBinPacking, bool NoLineBreak) : Indent(Indent), IndentLevel(IndentLevel), LastSpace(LastSpace), - FirstLessLess(0), BreakBeforeClosingBrace(false), QuestionColumn(0), + NestedBlockIndent(Indent), FirstLessLess(0), + BreakBeforeClosingBrace(false), QuestionColumn(0), AvoidBinPacking(AvoidBinPacking), BreakBeforeParameter(false), NoLineBreak(NoLineBreak), LastOperatorWrapped(true), ColonPos(0), StartOfFunctionCall(0), StartOfArraySubscripts(0), @@ -171,6 +172,10 @@ struct ParenState { /// OtherParameter)); unsigned LastSpace; + /// \brief If a block relative to this parenthesis level gets wrapped, indent + /// it this much. + unsigned NestedBlockIndent; + /// \brief The position the first "<<" operator encountered on each level. /// /// Used to align "<<" operators. 0 if no such operator has been encountered @@ -265,6 +270,8 @@ struct ParenState { return Indent < Other.Indent; if (LastSpace != Other.LastSpace) return LastSpace < Other.LastSpace; + if (NestedBlockIndent != Other.NestedBlockIndent) + return NestedBlockIndent < Other.NestedBlockIndent; if (FirstLessLess != Other.FirstLessLess) return FirstLessLess < Other.FirstLessLess; if (BreakBeforeClosingBrace != Other.BreakBeforeClosingBrace) diff --git a/clang/lib/Format/FormatToken.h b/clang/lib/Format/FormatToken.h index df47421..7ffbfbd 100644 --- a/clang/lib/Format/FormatToken.h +++ b/clang/lib/Format/FormatToken.h @@ -409,8 +409,9 @@ struct FormatToken { /// list that should be indented with a block indent. bool opensBlockTypeList(const FormatStyle &Style) const { return is(TT_ArrayInitializerLSquare) || - (is(tok::l_brace) && (BlockKind == BK_Block || is(TT_DictLiteral) || - !Style.Cpp11BracedListStyle)); + (is(tok::l_brace) && + (BlockKind == BK_Block || is(TT_DictLiteral) || + (!Style.Cpp11BracedListStyle && NestingLevel == 0))); } /// \brief Same as opensBlockTypeList, but for the closing token. diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp index fe9cf59..56feaf6 100644 --- a/clang/lib/Format/UnwrappedLineFormatter.cpp +++ b/clang/lib/Format/UnwrappedLineFormatter.cpp @@ -593,6 +593,15 @@ unsigned UnwrappedLineFormatter::analyzeSolutionSpace(LineState &InitialState, return Penalty; } +static void printLineState(const LineState &State) { + llvm::dbgs() << "State: "; + for (const ParenState &P : State.Stack) { + llvm::dbgs() << P.Indent << "|" << P.LastSpace << "|" << P.NestedBlockIndent + << " "; + } + llvm::dbgs() << State.NextToken->TokenText << "\n"; +} + void UnwrappedLineFormatter::reconstructPath(LineState &State, StateNode *Current) { std::deque Path; @@ -608,6 +617,7 @@ void UnwrappedLineFormatter::reconstructPath(LineState &State, Penalty += Indenter->addTokenToState(State, (*I)->NewLine, false); DEBUG({ + printLineState((*I)->Previous->State); if ((*I)->NewLine) { llvm::dbgs() << "Penalty for placing " << (*I)->Previous->State.NextToken->Tok.getName() << ": " @@ -648,13 +658,8 @@ bool UnwrappedLineFormatter::formatChildren(LineState &State, bool NewLine, return true; if (NewLine) { - int AdditionalIndent = - State.FirstIndent - State.Line->Level * Style.IndentWidth; - if (State.Stack.size() < 2 || - !State.Stack[State.Stack.size() - 2].NestedBlockInlined) { - AdditionalIndent = State.Stack.back().Indent - - Previous.Children[0]->Level * Style.IndentWidth; - } + int AdditionalIndent = State.Stack.back().Indent - + Previous.Children[0]->Level * Style.IndentWidth; Penalty += format(Previous.Children, DryRun, AdditionalIndent, /*FixBadIndentation=*/true); diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp index fdcfe79..2b4553c 100644 --- a/clang/unittests/Format/FormatTest.cpp +++ b/clang/unittests/Format/FormatTest.cpp @@ -5912,8 +5912,7 @@ TEST_F(FormatTest, LayoutCxx11BraceInitializers) { " BracedList{ // comment 1 (Forcing interesting break)\n" " param1, param2,\n" " // comment 2\n" - " param3, param4\n" - " });", + " param3, param4 });", ExtraSpaces); verifyFormat( "std::this_thread::sleep_for(\n" @@ -9421,6 +9420,12 @@ TEST_F(FormatTest, FormatsLambdas) { verifyFormat("void f() {\n" " MACRO((const AA &a) { return 1; });\n" "}"); + + verifyFormat("if (blah_blah(whatever, whatever, [] {\n" + " doo_dah();\n" + " doo_dah();\n" + " })) {\n" + "}"); } TEST_F(FormatTest, FormatsBlocks) { diff --git a/clang/unittests/Format/FormatTestJS.cpp b/clang/unittests/Format/FormatTestJS.cpp index 23654a88..1d12d32 100644 --- a/clang/unittests/Format/FormatTestJS.cpp +++ b/clang/unittests/Format/FormatTestJS.cpp @@ -206,14 +206,13 @@ TEST_F(FormatTestJS, FunctionLiterals) { " style: {direction: ''}\n" " }\n" "};"); - // FIXME: The formatting here probably isn't ideal. EXPECT_EQ("abc = xyz ?\n" " function() {\n" " return 1;\n" " } :\n" " function() {\n" - " return -1;\n" - "};", + " return -1;\n" + " };", format("abc=xyz?function(){return 1;}:function(){return -1;};")); verifyFormat("var closure = goog.bind(\n" @@ -254,6 +253,23 @@ TEST_F(FormatTestJS, FunctionLiterals) { " return 1;\n" " }\n" "};"); + verifyFormat("this.someObject.doSomething(aaaaaaaaaaaaaaaaaaaaaaaaaa)\n" + " .then(goog.bind(function(aaaaaaaaaaa) {\n" + " someFunction();\n" + " someFunction();\n" + " }, this), aaaaaaaaaaaaaaaaa);"); + + // FIXME: This is not ideal yet. + verifyFormat("someFunction(goog.bind(\n" + " function() {\n" + " doSomething();\n" + " doSomething();\n" + " },\n" + " this),\n" + " goog.bind(function() {\n" + " doSomething();\n" + " doSomething();\n" + " }, this));"); } TEST_F(FormatTestJS, InliningFunctionLiterals) { @@ -341,7 +357,10 @@ TEST_F(FormatTestJS, MultipleFunctionLiterals) { verifyFormat("getSomeLongPromise()\n" " .then(function(value) { body(); })\n" - " .thenCatch(function(error) { body(); });"); + " .thenCatch(function(error) {\n" + " body();\n" + " body();\n" + " });"); verifyFormat("getSomeLongPromise()\n" " .then(function(value) {\n" " body();\n" @@ -351,6 +370,11 @@ TEST_F(FormatTestJS, MultipleFunctionLiterals) { " body();\n" " body();\n" " });"); + + // FIXME: This is bad, but it used to be formatted correctly by accident. + verifyFormat("getSomeLongPromise().then(function(value) {\n" + " body();\n" + "}).thenCatch(function(error) { body(); });"); } TEST_F(FormatTestJS, ReturnStatements) { -- 2.7.4