From 11e13801d8a25cea011c2a154380c371b6ddaaf6 Mon Sep 17 00:00:00 2001 From: Daniel Jasper Date: Wed, 8 May 2013 14:12:04 +0000 Subject: [PATCH] Improve line breaking in binary expressions. If the LHS of a binary expression is broken, clang-format should also break after the operator as otherwise: - The RHS can be easy to miss - It can look as if clang-format doesn't understand operator precedence Before: bool aaaaaaaaaaaaaaaaaaaaa = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa != bbbbbbbbbbbbbbbbbb && ccccccccc == ddddddddddd; After: bool aaaaaaaaaaaaaaaaaaaaa = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa != bbbbbbbbbbbbbbbbbb && ccccccccc == ddddddddddd; As an additional note, clang-format would also be ok with the following formatting, it just has a higher penalty (IMO correctly so). bool aaaaaaaaaaaaaaaaaaaaa = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa != bbbbbbbbbbbbbbbbbb && ccccccccc == ddddddddddd; git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@181430 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Format/Format.cpp | 62 +++++++++++++++++++-------------- lib/Format/TokenAnnotator.cpp | 5 +-- unittests/Format/FormatTest.cpp | 18 ++++++++++ 3 files changed, 56 insertions(+), 29 deletions(-) diff --git a/lib/Format/Format.cpp b/lib/Format/Format.cpp index 814017a512..2413e43b13 100644 --- a/lib/Format/Format.cpp +++ b/lib/Format/Format.cpp @@ -467,8 +467,9 @@ private: if (Current.is(tok::question)) State.Stack.back().BreakBeforeParameter = true; - if (Previous.isOneOf(tok::comma, tok::semi) && - !State.Stack.back().AvoidBinPacking) + if ((Previous.isOneOf(tok::comma, tok::semi) && + !State.Stack.back().AvoidBinPacking) || + Previous.Type == TT_BinaryOperator) State.Stack.back().BreakBeforeParameter = false; if (!DryRun) { @@ -495,7 +496,7 @@ private: } const AnnotatedToken *TokenBefore = Current.getPreviousNoneComment(); if (TokenBefore && !TokenBefore->isOneOf(tok::comma, tok::semi) && - !TokenBefore->opensScope()) + TokenBefore->Type != TT_BinaryOperator && !TokenBefore->opensScope()) State.Stack.back().BreakBeforeParameter = true; // If we break after {, we should also break before the corresponding }. @@ -565,9 +566,9 @@ private: State.Stack.back().LastSpace = State.Column; else if (Previous.Type == TT_InheritanceColon) State.Stack.back().Indent = State.Column; - else if (Previous.opensScope() && Previous.ParameterCount > 1) - // If this function has multiple parameters, indent nested calls from - // the start of the first parameter. + else if (Previous.opensScope() && !Current.FakeLParens.empty()) + // If this function has multiple parameters or a binary expression + // parameter, indent nested calls from the start of the first parameter. State.Stack.back().LastSpace = State.Column; } @@ -906,40 +907,45 @@ private: /// \brief Returns \c true, if a line break after \p State is mandatory. bool mustBreak(const LineState &State) { - if (State.NextToken->MustBreakBefore) + const AnnotatedToken &Current = *State.NextToken; + const AnnotatedToken &Previous = *Current.Parent; + if (Current.MustBreakBefore || Current.Type == TT_InlineASMColon) return true; - if (State.NextToken->is(tok::r_brace) && - State.Stack.back().BreakBeforeClosingBrace) + if (Current.is(tok::r_brace) && State.Stack.back().BreakBeforeClosingBrace) return true; - if (State.NextToken->Parent->is(tok::semi) && - State.LineContainsContinuedForLoopSection) + if (Previous.is(tok::semi) && State.LineContainsContinuedForLoopSection) return true; - if ((State.NextToken->Parent->isOneOf(tok::comma, tok::semi) || - State.NextToken->is(tok::question) || - State.NextToken->Type == TT_ConditionalExpr) && + if ((Previous.isOneOf(tok::comma, tok::semi) || Current.is(tok::question) || + Current.Type == TT_ConditionalExpr) && State.Stack.back().BreakBeforeParameter && - !State.NextToken->isTrailingComment() && - State.NextToken->isNot(tok::r_paren) && - State.NextToken->isNot(tok::r_brace)) + !Current.isTrailingComment() && + !Current.isOneOf(tok::r_paren, tok::r_brace)) + return true; + + // If we need to break somewhere inside the LHS of a binary expression, we + // should also break after the operator. + if (Previous.Type == TT_BinaryOperator && + !Previous.isOneOf(tok::lessless, tok::question) && + getPrecedence(Previous) != prec::Assignment && + State.Stack.back().BreakBeforeParameter) return true; + // FIXME: Comparing LongestObjCSelectorName to 0 is a hacky way of finding // out whether it is the first parameter. Clean this up. - if (State.NextToken->Type == TT_ObjCSelectorName && - State.NextToken->LongestObjCSelectorName == 0 && + if (Current.Type == TT_ObjCSelectorName && + Current.LongestObjCSelectorName == 0 && State.Stack.back().BreakBeforeParameter) return true; - if ((State.NextToken->Type == TT_CtorInitializerColon || - (State.NextToken->Parent->ClosesTemplateDeclaration && - State.ParenLevel == 0))) - return true; - if (State.NextToken->Type == TT_InlineASMColon) + if ((Current.Type == TT_CtorInitializerColon || + (Previous.ClosesTemplateDeclaration && State.ParenLevel == 0))) return true; + // This prevents breaks like: // ... // SomeParameter, OtherParameter).DoSomething( // ... // As they hide "DoSomething" and generally bad for readability. - if (State.NextToken->isOneOf(tok::period, tok::arrow) && + if (Current.isOneOf(tok::period, tok::arrow) && getRemainingLength(State) + State.Column > getColumnLimit() && State.ParenLevel < State.StartOfLineLevel) return true; @@ -1166,8 +1172,10 @@ public: Lex.MeasureTokenLength(LastLoc, SourceMgr, Lex.getLangOpts()) - 1; PreviousLineWasTouched = false; if (TheLine.Last->is(tok::comment)) - Whitespaces.addUntouchableComment(SourceMgr.getSpellingColumnNumber( - TheLine.Last->FormatTok.Tok.getLocation()) - 1); + Whitespaces.addUntouchableComment( + SourceMgr.getSpellingColumnNumber( + TheLine.Last->FormatTok.Tok.getLocation()) - + 1); else Whitespaces.alignComments(); } diff --git a/lib/Format/TokenAnnotator.cpp b/lib/Format/TokenAnnotator.cpp index 3ac5a07884..c736e9012f 100644 --- a/lib/Format/TokenAnnotator.cpp +++ b/lib/Format/TokenAnnotator.cpp @@ -320,7 +320,7 @@ private: Tok->Type = TT_ObjCMethodExpr; Tok->Parent->Type = TT_ObjCSelectorName; if (Tok->Parent->FormatTok.TokenLength > - Contexts.back().LongestObjCSelectorName) + Contexts.back().LongestObjCSelectorName) Contexts.back().LongestObjCSelectorName = Tok->Parent->FormatTok.TokenLength; if (Contexts.back().FirstObjCSelectorName == NULL) @@ -606,7 +606,8 @@ private: if (Current.Parent && Current.is(tok::identifier) && ((Current.Parent->is(tok::identifier) && Current.Parent->FormatTok.Tok.getIdentifierInfo() - ->getPPKeywordID() == tok::pp_not_keyword) || + ->getPPKeywordID() == + tok::pp_not_keyword) || isSimpleTypeSpecifier(*Current.Parent) || Current.Parent->Type == TT_PointerOrReference || Current.Parent->Type == TT_TemplateCloser)) { diff --git a/unittests/Format/FormatTest.cpp b/unittests/Format/FormatTest.cpp index 2a11f80eaa..3a97abe5b2 100644 --- a/unittests/Format/FormatTest.cpp +++ b/unittests/Format/FormatTest.cpp @@ -1606,6 +1606,24 @@ TEST_F(FormatTest, PreventConfusingIndents) { " ddd);"); } +TEST_F(FormatTest, LineBreakingInBinaryExpressions) { + verifyFormat( + "bool aaaaaaa =\n" + " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaa).aaaaaaaaaaaaaaaaaaa() ||\n" + " bbbbbbbb();"); + verifyFormat("bool aaaaaaaaaaaaaaaaaaaaa =\n" + " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa != bbbbbbbbbbbbbbbbbb &&\n" + " ccccccccc == ddddddddddd;"); + + verifyFormat("aaaaaa = aaaaaaa(aaaaaaa, // break\n" + " aaaaaa) &&\n" + " bbbbbb && cccccc;"); + verifyFormat("Whitespaces.addUntouchableComment(\n" + " SourceMgr.getSpellingColumnNumber(\n" + " TheLine.Last->FormatTok.Tok.getLocation()) -\n" + " 1);"); +} + TEST_F(FormatTest, ExpressionIndentation) { verifyFormat("bool value = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +\n" " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +\n" -- 2.40.0