[clang-format] Look at NoLineBreak and NoLineBreakInOperand before breakProtrudingToken

[krasimir]

Summary

This patch makes ContinuationIndenter call breakProtrudingToken only if
NoLineBreak and NoLineBreakInOperand is false.

Previously, clang-format required two runs to converge on the following example with 24 columns:
Note that the second operand shouldn't be splitted according to NoLineBreakInOperand, but the
token breaker doesn't take that into account:

func(a, "long long long long", c);

After first run:

func(a, "long long "
        "long long",
         c);

After second run, where NoLineBreakInOperand is taken into account:

func(a,
     "long long "
     "long long",
     c);

With the patch, clang-format now obtains in one run:

func(a,
     "long long long"
     "long",
     c);

which is a better token split overall.

[krasimir]

• Remove file

[djasper]

lib/Format/ContinuationIndenter.cpp

852

I think we assume here and in many other place that the stack is never empty. E.g. there is no similar check in l.847 above. So, I'd remove this here, too. In the long run, we probably want to have a class properly wrapping the Stack.

[krasimir]

• Remove empty stack check

[djasper]

Looks good.

2

unsigned ContinuationIndenter::addTokenToState(LineState &State, bool Newline,
 
   return moveStateToNextToken(State, DryRun, Newline) + Penalty;
 }
 
 void ContinuationIndenter::addTokenOnCurrentLine(LineState &State, bool DryRun,
                                                  unsigned ExtraSpaces) {
   FormatToken &Current = *State.NextToken;
   const FormatToken &Previous = *State.NextToken->Previous;
+  DEBUG({
+    llvm::dbgs() << "addTokenOnCurrentLine\n";
+    llvm::dbgs() << "  Previous = " << Previous.TokenText << "\n";
+    llvm::dbgs() << "  Current = " << Current.TokenText << "\n";
+  });
   if (Current.is(tok::equal) &&
       (State.Line->First->is(tok::kw_for) || Current.NestingLevel == 0) &&
       State.Stack.back().VariablePos == 0) {
     State.Stack.back().VariablePos = State.Column;
     // Move over * and & if they are bound to the variable name.
     const FormatToken *Tok = &Previous;
     while (Tok && State.Stack.back().VariablePos >= Tok->ColumnWidth) {
       State.Stack.back().VariablePos -= Tok->ColumnWidth;
if (Style.AlignAfterOpenBracket == FormatStyle::BAS_AlwaysBreak &&
       // Don't do this for simple (no expressions) one-argument function calls
       // as that feels like needlessly wasting whitespace, e.g.:
       //
       //   caaaaaaaaaaaall(
       //       caaaaaaaaaaaall(
       //           caaaaaaaaaaaall(
       //               caaaaaaaaaaaaaaaaaaaaaaall(aaaaaaaaaaaaaa, aaaaaaaaa))));
       Current.FakeLParens.size() > 0 &&
-      Current.FakeLParens.back() > prec::Unknown)
+      Current.FakeLParens.back() > prec::Unknown) {
+    DEBUG({
+      llvm::dbgs() << ":386\n";
+    });
     State.Stack.back().NoLineBreak = true;
+  }
   if (Previous.is(TT_TemplateString) && Previous.opensScope())
     State.Stack.back().NoLineBreak = true;
 
   if (Style.AlignAfterOpenBracket != FormatStyle::BAS_DontAlign &&
       Previous.opensScope() && Previous.isNot(TT_ObjCMethodExpr) &&
       (Current.isNot(TT_LineComment) || Previous.BlockKind == BK_BracedInit))
     State.Stack.back().Indent = State.Column + Spaces;
   if (State.Stack.back().AvoidBinPacking && startsNextParameter(Current, Style))
unsigned ContinuationIndenter::moveStateToNextToken(LineState &State,
 
   moveStatePastFakeLParens(State, Newline);
   moveStatePastScopeCloser(State);
   if (Current.is(TT_TemplateString) && Current.opensScope())
     State.Stack.back().LastSpace =
         (Current.IsMultiline ? Current.LastLineColumnWidth
                              : State.Column + Current.ColumnWidth) -
         strlen("${");
+  bool CanBreakProtrudingToken =
+      State.Stack.empty() || (!State.Stack.back().NoLineBreak &&
+                              !State.Stack.back().NoLineBreakInOperand);
   moveStatePastScopeOpener(State, Newline);
   moveStatePastFakeRParens(State);
 
   if (Current.isStringLiteral() && State.StartOfStringLiteral == 0)
     State.StartOfStringLiteral = State.Column;
   if (Current.is(TT_ObjCStringLiteral) && State.StartOfStringLiteral == 0)
     State.StartOfStringLiteral = State.Column + 1;
   else if (!Current.isOneOf(tok::comment, tok::identifier, tok::hash) &&
            !Current.isStringLiteral())
     State.StartOfStringLiteral = 0;
 
   State.Column += Current.ColumnWidth;
   State.NextToken = State.NextToken->Next;
-  unsigned Penalty = breakProtrudingToken(Current, State, DryRun);
+  unsigned Penalty = 0;
+  if (CanBreakProtrudingToken)
+    Penalty = breakProtrudingToken(Current, State, DryRun);
   if (State.Column > getColumnLimit(State)) {
     unsigned ExcessCharacters = State.Column - getColumnLimit(State);
     Penalty += Style.PenaltyExcessCharacter * ExcessCharacters;
   }
 
   if (Current.Role)
     Current.Role->formatFromToken(State, this, DryRun);
   // If the previous has a special role, let it consume tokens as appropriate.

lib/Format/ContinuationIndenter.cpp

unsigned ContinuationIndenter::moveStateToNextToken(LineState &State,
 
   moveStatePastFakeLParens(State, Newline);
   moveStatePastScopeCloser(State);
   if (Current.is(TT_TemplateString) && Current.opensScope())
     State.Stack.back().LastSpace =
         (Current.IsMultiline ? Current.LastLineColumnWidth
                              : State.Column + Current.ColumnWidth) -
         strlen("${");
+  bool CanBreakProtrudingToken =
+      State.Stack.empty() || (!State.Stack.back().NoLineBreak &&
+                              !State.Stack.back().NoLineBreakInOperand);
   moveStatePastScopeOpener(State, Newline);
   moveStatePastFakeRParens(State);
 
   if (Current.isStringLiteral() && State.StartOfStringLiteral == 0)
     State.StartOfStringLiteral = State.Column;
   if (Current.is(TT_ObjCStringLiteral) && State.StartOfStringLiteral == 0)
     State.StartOfStringLiteral = State.Column + 1;
   else if (!Current.isOneOf(tok::comment, tok::identifier, tok::hash) &&
            !Current.isStringLiteral())
     State.StartOfStringLiteral = 0;
 
   State.Column += Current.ColumnWidth;
   State.NextToken = State.NextToken->Next;
-  unsigned Penalty = breakProtrudingToken(Current, State, DryRun);
+  unsigned Penalty = 0;
+  if (CanBreakProtrudingToken)
+    Penalty = breakProtrudingToken(Current, State, DryRun);
   if (State.Column > getColumnLimit(State)) {
     unsigned ExcessCharacters = State.Column - getColumnLimit(State);
     Penalty += Style.PenaltyExcessCharacter * ExcessCharacters;
   }
 
   if (Current.Role)
     Current.Role->formatFromToken(State, this, DryRun);
   // If the previous has a special role, let it consume tokens as appropriate.

unittests/Format/FormatTest.cpp

 
 TEST_F(FormatTest, BreaksStringLiteralsWithin_TMacro) {
   FormatStyle Style = getLLVMStyleWithColumns(20);
   EXPECT_EQ(
       "_T(\"aaaaaaaaaaaaaa\")\n"
       "_T(\"aaaaaaaaaaaaaa\")\n"
       "_T(\"aaaaaaaaaaaa\")",
       format("  _T(\"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\")", Style));
-  EXPECT_EQ("f(x, _T(\"aaaaaaaaa\")\n"
-            "     _T(\"aaaaaa\"),\n"
+  EXPECT_EQ("f(x,\n"
+            "  _T(\"aaaaaaaaaaaa\")\n"
+            "  _T(\"aaa\"),\n"
             "  z);",
             format("f(x, _T(\"aaaaaaaaaaaaaaa\"), z);", Style));
 
   // FIXME: Handle embedded spaces in one iteration.
   //  EXPECT_EQ("_T(\"aaaaaaaaaaaaa\")\n"
   //            "_T(\"aaaaaaaaaaaaa\")\n"
   //            "_T(\"aaaaaaaaaaaaa\")\n"
   //            "_T(\"a\")",
TEST_F(FormatTest, BreaksStringLiteralsWithin_TMacro) {
   EXPECT_EQ("f(\n"
             "\n"
             "    _T(\"XXXXXXXXXXXXXXXXXXXXXXXXXXXXXn\"));",
             format("f(\n"
                    "\n"
                    "_T(\"XXXXXXXXXXXXXXXXXXXXXXXXXXXXXn\"));"));
 }
 
+TEST_F(FormatTest, BreaksStringLiteralOperands) {
+  // In a function call with two operands, the second can be broken with no line
+  // break before it.
+  EXPECT_EQ("func(a, \"long long \"\n"
+            "        \"long long\");",
+            format("func(a, \"long long long long\");",
+                   getLLVMStyleWithColumns(24)));
+  // In a function call with three operands, the second must be broken with a
+  // line break before it.
+  EXPECT_EQ("func(a,\n"
+            "     \"long long long \"\n"
+            "     \"long\",\n"
+            "     c);",
+            format("func(a, \"long long long long\", c);",
+                   getLLVMStyleWithColumns(24)));
+  // In a function call with three operands, the third must be broken with a
+  // line break before it.
+  EXPECT_EQ("func(a, b,\n"
+            "     \"long long long \"\n"
+            "     \"long\");",
+            format("func(a, b, \"long long long long\");",
+                   getLLVMStyleWithColumns(24)));
+  // In a function call with three operands, both the second and the third must
+  // be broken with a line break before them.
+  EXPECT_EQ("func(a,\n"
+            "     \"long long long \"\n"
+            "     \"long\",\n"
+            "     \"long long long \"\n"
+            "     \"long\");",
+            format("func(a, \"long long long long\", \"long long long long\");",
+                   getLLVMStyleWithColumns(24)));
+  // In a chain of << with two operands, the second can be broken with no line
+  // break before it.
+  EXPECT_EQ("a << \"line line \"\n"
+            "     \"line\";",
+            format("a << \"line line line\";",
+                   getLLVMStyleWithColumns(20)));
+  // In a chain of << with three operands, the second can be broken with no line
+  // break before it.
+  EXPECT_EQ("abcde << \"line \"\n"
+            "         \"line line\"\n"
+            "      << c;",
+            format("abcde << \"line line line\" << c;",
+                   getLLVMStyleWithColumns(20)));
+  // In a chain of << with three operands, the third must be broken with a line
+  // break before it.
+  EXPECT_EQ("a << b\n"
+            "  << \"line line \"\n"
+            "     \"line\";",
+            format("a << b << \"line line line\";",
+                   getLLVMStyleWithColumns(20)));
+  // In a chain of << with three operands, the second can be broken with no line
+  // break before it and the third must be broken with a line break before it.
+  EXPECT_EQ("abcd << \"line line \"\n"
+            "        \"line\"\n"
+            "     << \"line line \"\n"
+            "        \"line\";",
+            format("abcd << \"line line line\" << \"line line line\";",
+                   getLLVMStyleWithColumns(20)));
+  // In a chain of binary operators with two operands, the second can be broken
+  // with no line break before it.
+  EXPECT_EQ("abcd + \"line line \"\n"
+            "       \"line line\";",
+            format("abcd + \"line line line line\";",
+                   getLLVMStyleWithColumns(20)));
+  // In a chain of binary operators with three operands, the second must be
+  // broken with a line break before it.
+  EXPECT_EQ("abcd +\n"
+            "    \"line line \"\n"
+            "    \"line line\" +\n"
+            "    e;",
+            format("abcd + \"line line line line\" + e;",
+                   getLLVMStyleWithColumns(20)));
+  // In a function call with two operands, with AlignAfterOpenBracket enabled,
+  // the first must be broken with a line break before it.
+  FormatStyle Style = getLLVMStyleWithColumns(25);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  EXPECT_EQ("someFunction(\n"
+            "    \"long long long \"\n"
+            "    \"long\",\n"
+            "    a);",
+            format("someFunction(\"long long long long\", a);", Style));
+}
+
 TEST_F(FormatTest, DontSplitStringLiteralsWithEscapedNewlines) {
   EXPECT_EQ(
       "aaaaaaaaaaa = \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\\\n"
       "  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\\\n"
       "  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\";",
       format("aaaaaaaaaaa  =  \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\\\n"
              "  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\\\n"
              "  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\";"));