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;

llvm-svn: 181430

clang/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;
-    // 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 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;
-    if ((State.NextToken->Type == TT_CtorInitializerColon ||
-         (State.NextToken->Parent->ClosesTemplateDeclaration &&
-          State.ParenLevel == 0)))
+
+    // FIXME: Comparing LongestObjCSelectorName to 0 is a hacky way of finding
+    // out whether it is the first parameter. Clean this up.
+    if (Current.Type == TT_ObjCSelectorName &&
+        Current.LongestObjCSelectorName == 0 &&
+        State.Stack.back().BreakBeforeParameter)
       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();
       }

clang/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)) {

clang/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"