Add option to avoid "bin-packing" of parameters.

"Bin-packing" here means allowing multiple parameters on one line, if a
function call/declaration is spread over multiple lines.

This is required by the Chromium style guide and probably desired for
the Google style guide. Not making changes to LLVM style as I don't have
enough data.

With this enabled, we format stuff like:
aaaaaaaaaaaaaaa(aaaaaaaaaa,
                aaaaaaaaaa,
		aaaaaaaaaaaaaaaaaaaaaaa).aaaaaaaaaaaaaaaaaa();

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@172617 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/Format/Format.h

@@ -57,6 +57,10 @@ struct FormatStyle {
   /// \brief The number of spaces to before trailing line comments.
   unsigned SpacesBeforeTrailingComments;
 
+  /// \brief If false, a function call's or function definition's parameters
+  /// will either all be on the same line or will have one line each.
+  bool BinPackParameters;
+
   /// \brief If the constructor initializers don't fit on a line, put each
   /// initializer on its own line.
   bool ConstructorInitializerAllOnOneLineOrOnePerLine;

lib/Format/Format.cpp

@@ -73,7 +73,7 @@ public:
   AnnotatedToken(const FormatToken &FormatTok)
       : FormatTok(FormatTok), Type(TT_Unknown), SpaceRequiredBefore(false),
         CanBreakBefore(false), MustBreakBefore(false),
-        ClosesTemplateDeclaration(false), Parent(NULL) {}
+        ClosesTemplateDeclaration(false), MatchingParen(NULL), Parent(NULL) {}
 
   bool is(tok::TokenKind Kind) const { return FormatTok.Tok.is(Kind); }
   bool isNot(tok::TokenKind Kind) const { return FormatTok.Tok.isNot(Kind); }
@@ -92,6 +92,8 @@ public:
 
   bool ClosesTemplateDeclaration;
 
+  AnnotatedToken *MatchingParen;
+
   /// \brief The total length of the line up to and including this token.
   unsigned TotalLength;
 
@@ -146,6 +148,7 @@ FormatStyle getLLVMStyle() {
   LLVMStyle.SplitTemplateClosingGreater = true;
   LLVMStyle.IndentCaseLabels = false;
   LLVMStyle.SpacesBeforeTrailingComments = 1;
+  LLVMStyle.BinPackParameters = true;
   LLVMStyle.ConstructorInitializerAllOnOneLineOrOnePerLine = false;
   LLVMStyle.AllowShortIfStatementsOnASingleLine = false;
   LLVMStyle.ObjCSpaceBeforeProtocolList = true;
@@ -162,6 +165,7 @@ FormatStyle getGoogleStyle() {
   GoogleStyle.SplitTemplateClosingGreater = false;
   GoogleStyle.IndentCaseLabels = true;
   GoogleStyle.SpacesBeforeTrailingComments = 2;
+  GoogleStyle.BinPackParameters = false;
   GoogleStyle.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
   GoogleStyle.AllowShortIfStatementsOnASingleLine = true;
   GoogleStyle.ObjCSpaceBeforeProtocolList = false;
@@ -318,7 +322,8 @@ private:
   struct ParenState {
     ParenState(unsigned Indent, unsigned LastSpace)
         : Indent(Indent), LastSpace(LastSpace), FirstLessLess(0),
-          BreakBeforeClosingBrace(false), BreakAfterComma(false) {}
+          BreakBeforeClosingBrace(false), BreakAfterComma(false),
+          HasMultiParameterLine(false) {}
 
     /// \brief The position to which a specific parenthesis level needs to be
     /// indented.
@@ -345,6 +350,7 @@ private:
     bool BreakBeforeClosingBrace;
 
     bool BreakAfterComma;
+    bool HasMultiParameterLine;
 
     bool operator<(const ParenState &Other) const {
       if (Indent != Other.Indent)
@@ -357,6 +363,8 @@ private:
         return BreakBeforeClosingBrace;
       if (BreakAfterComma != Other.BreakAfterComma)
         return BreakAfterComma;
+      if (HasMultiParameterLine != Other.HasMultiParameterLine)
+        return HasMultiParameterLine;
       return false;
     }
   };
@@ -484,6 +492,9 @@ private:
       if (Previous.is(tok::l_paren) || Previous.is(tok::l_brace) ||
           State.NextToken->Parent->Type == TT_TemplateOpener)
         State.Stack[ParenLevel].Indent = State.Column + Spaces;
+      if (Previous.is(tok::comma))
+        State.Stack[ParenLevel].HasMultiParameterLine = true;
+
 
       // Top-level spaces that are not part of assignments are exempt as that
       // mostly leads to better results.
@@ -492,9 +503,18 @@ private:
           (ParenLevel != 0 || getPrecedence(Previous) == prec::Assignment))
         State.Stack[ParenLevel].LastSpace = State.Column;
     }
-    if (Newline && Previous.is(tok::l_brace)) {
+
+    // 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 we are breaking after '(', '{', '<' or ',', we need to break after
+    // future commas as well to avoid bin packing.
+    if (!Style.BinPackParameters && Newline &&
+        (Previous.is(tok::comma) || Previous.is(tok::l_paren) ||
+         Previous.is(tok::l_brace) || Previous.Type == TT_TemplateOpener))
+      State.Stack.back().BreakAfterComma = true;
+
     moveStateToNextToken(State);
   }
 
@@ -523,6 +543,17 @@ private:
       }
       State.Stack.push_back(
           ParenState(NewIndent, State.Stack.back().LastSpace));
+
+      // If the entire set of parameters will not fit on the current line, we
+      // will need to break after commas on this level to avoid bin-packing.
+      if (!Style.BinPackParameters && Current.MatchingParen != NULL &&
+          !Current.Children.empty()) {
+        if (getColumnLimit() < State.Column + Current.FormatTok.TokenLength +
+            Current.MatchingParen->TotalLength -
+            Current.Children[0].TotalLength) {
+          State.Stack.back().BreakAfterComma = true;
+        }
+      }
     }
 
     // If we encounter a closing ), ], } or >, we can remove a level from our
@@ -623,6 +654,11 @@ private:
         State.NextToken->Type != TT_LineComment &&
         State.Stack.back().BreakAfterComma)
       return UINT_MAX;
+    // Trying to insert a parameter on a new line if there are already more than
+    // one parameter on the current line is bin packing.
+    if (NewLine && State.NextToken->Parent->is(tok::comma) &&
+        State.Stack.back().HasMultiParameterLine && !Style.BinPackParameters)
+      return UINT_MAX;
     if (!NewLine && State.NextToken->Type == TT_CtorInitializerColon)
       return UINT_MAX;
 
@@ -631,7 +667,8 @@ private:
       CurrentPenalty += Parameters.PenaltyIndentLevel * State.Stack.size() +
                         splitPenalty(*State.NextToken->Parent);
     } else {
-      if (State.Stack.size() < State.StartOfLineLevel)
+      if (State.Stack.size() < State.StartOfLineLevel &&
+          State.NextToken->is(tok::identifier))
         CurrentPenalty += Parameters.PenaltyLevelDecrease *
                           (State.StartOfLineLevel - State.Stack.size());
     }
@@ -706,8 +743,13 @@ public:
           ColonIsObjCMethodExpr(false) {}
 
     bool parseAngle() {
+      if (CurrentToken == NULL)
+        return false;
+      AnnotatedToken *Left = CurrentToken->Parent;
       while (CurrentToken != NULL) {
         if (CurrentToken->is(tok::greater)) {
+          Left->MatchingParen = CurrentToken;
+          CurrentToken->MatchingParen = Left;
           CurrentToken->Type = TT_TemplateCloser;
           next();
           return true;
@@ -725,10 +767,15 @@ public:
     }
 
     bool parseParens() {
-      if (CurrentToken != NULL && CurrentToken->is(tok::caret))
-        CurrentToken->Parent->Type = TT_ObjCBlockLParen;
+      if (CurrentToken == NULL)
+        return false;
+      AnnotatedToken *Left = CurrentToken->Parent;
+      if (CurrentToken->is(tok::caret))
+        Left->Type = TT_ObjCBlockLParen;
       while (CurrentToken != NULL) {
         if (CurrentToken->is(tok::r_paren)) {
+          Left->MatchingParen = CurrentToken;
+          CurrentToken->MatchingParen = Left;
           next();
           return true;
         }
@@ -781,8 +828,14 @@ public:
     }
 
     bool parseBrace() {
+      // Lines are fine to end with '{'.
+      if (CurrentToken == NULL)
+        return true;
+      AnnotatedToken *Left = CurrentToken->Parent;
       while (CurrentToken != NULL) {
         if (CurrentToken->is(tok::r_brace)) {
+          Left->MatchingParen = CurrentToken;
+          CurrentToken->MatchingParen = Left;
           next();
           return true;
         }
@@ -791,7 +844,6 @@ public:
         if (!consumeToken())
           return false;
       }
-      // Lines can currently end with '{'.
       return true;
     }
 

unittests/Format/FormatTest.cpp

@@ -484,10 +484,11 @@ TEST_F(FormatTest, StaticInitializers) {
 TEST_F(FormatTest, NestedStaticInitializers) {
   verifyFormat("static A x = { { {} } };\n");
   verifyFormat(
-      "static A x = {\n"
-      "  { { init1, init2, init3, init4 }, { init1, init2, init3, init4 } }\n"
-      "};\n");
-  verifyFormat(
+      "static A x = { { { init1, init2, init3, init4 },\n"
+      "                 { init1, init2, init3, init4 } } };");
+
+  // FIXME: Fix this in general an verify that it works in LLVM style again.
+  verifyGoogleFormat(
       "somes Status::global_reps[3] = {\n"
       "  { kGlobalRef, OK_CODE, NULL, NULL, NULL },\n"
       "  { kGlobalRef, CANCELLED_CODE, NULL, NULL, NULL },\n"
@@ -670,8 +671,7 @@ TEST_F(FormatTest, LayoutStatementsAroundPreprocessorDirectives) {
       "#define A B\n"
       "                       withMoreParamters,\n"
       "                       whichStronglyInfluenceTheLayout),\n"
-      "    andMoreParameters),\n"
-      "               trailing);", getLLVMStyleWithColumns(69));
+      "    andMoreParameters), trailing);", getLLVMStyleWithColumns(69));
 }
 
 TEST_F(FormatTest, LayoutBlockInsideParens) {
@@ -763,22 +763,13 @@ TEST_F(FormatTest, ConstructorInitializers) {
                      "}");
 
   // This test takes VERY long when memoization is broken.
-  verifyGoogleFormat(
-      "Constructor()\n"
-      "    : aaaa(a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a,"
-      " a, a, a,\n"
-      "           a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a,"
-      " a, a, a,\n"
-      "           a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a,"
-      " a, a, a,\n"
-      "           a, a, a, a, a, a, a, a, a, a, a)\n"
-      "      aaaa(a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a,"
-      " a, a, a,\n"
-      "           a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a,"
-      " a, a, a,\n"
-      "           a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a,"
-      " a, a, a,\n"
-      "           a, a, a, a, a, a, a, a, a, a, a) {}\n");
+  std::string input = "Constructor()\n"
+                 "    : aaaa(a,\n";
+  for (unsigned i = 0, e = 80; i != e; ++i) {
+    input += "           a,\n";
+  }
+  input += "           a) {}";
+  verifyGoogleFormat(input);
 }
 
 TEST_F(FormatTest, BreaksAsHighAsPossible) {
@@ -829,6 +820,31 @@ TEST_F(FormatTest, BreaksDesireably) {
                "    }\n  }\n}");
 }
 
+TEST_F(FormatTest, FormatsOneParameterPerLineIfNecessary) {
+  verifyGoogleFormat(
+      "aaaaaaaa(aaaaaaaaaaaaa,\n"
+      "         aaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
+      "             aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa)),\n"
+      "         aaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
+      "             aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa)));");
+  verifyGoogleFormat(
+      "aaaaaaaaaaaaaaa(aaaaaaaaa,\n"
+      "                aaaaaaaaa,\n"
+      "                aaaaaaaaaaaaaaaaaaaaaaa).aaaaaaaaaaaaaaaaaa();");
+  verifyGoogleFormat(
+      "somefunction(someotherFunction(ddddddddddddddddddddddddddddddddddd,\n"
+      "                               ddddddddddddddddddddddddddddd),\n"
+      "             test);");
+
+  verifyGoogleFormat(
+      "std::vector<aaaaaaaaaaaaaaaaaaaaaaa,\n"
+      "            aaaaaaaaaaaaaaaaaaaaaaa,\n"
+      "            aaaaaaaaaaaaaaaaaaaaaaa> aaaaaaaaaaaaaaaaaa;");
+  verifyGoogleFormat("a(\"a\"\n"
+                     "  \"a\",\n"
+                     "  a);");
+}
+
 TEST_F(FormatTest, DoesNotBreakTrailingAnnotation) {
   verifyFormat("void aaaaaaaaaaaa(int aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa)\n"
                "    GUARDED_BY(aaaaaaaaaaaaa);");