Add experimental flag for adaptive parameter bin-packing.

This is not activated for any style, might change or go away
completely.

For those that want to play around with it, set
ExperimentalAutoDetectBinPacking to true.

clang-format will then:
Look at whether function calls/declarations/definitions are currently
formatted with one parameter per line (on a case-by-case basis). If so,
clang-format will avoid bin-packing the parameters. If all parameters
are on one line (thus that line is "inconclusive"), clang-format will
make the choice dependent on whether there are other bin-packed
calls/declarations in the same file.

The reason for this change is that bin-packing in some situations can be
really bad and an author might opt to put one parameter on each line. If
the author does that, he might want clang-format not to mess with that.
If the author is unhappy with the one-per-line formatting, clang-format
can easily be convinced to bin-pack by putting any two parameters on the
same line.

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

diff --git a/include/clang/Format/Format.h b/include/clang/Format/Format.h
index ab3c1aa3328bc25bab4625e0d4f6a90121edd5c9..f8786c85557d8ff9f5ad14f52d4f4df8ab832004 100644 (file)
--- a/include/clang/Format/Format.h
+++ b/include/clang/Format/Format.h
@@ -77,6 +77,18 @@ struct FormatStyle {
   /// will either all be on the same line or will have one line each.
   bool BinPackParameters;
 
+  /// \brief If true, clang-format detects whether function calls and
+  /// definitions are formatted with one parameter per line.
+  ///
+  /// Each call can be bin-packed, one-per-line or inconclusive. If it is
+  /// inconclusive, e.g. completely on one line, but a decision needs to be
+  /// made, clang-format analyzes whether there are other bin-packed cases in
+  /// the input file and act accordingly.
+  ///
+  /// NOTE: This is an experimental flag, that might go away or be renamed. Do
+  /// not use this in config files, etc. Use at your own risk.
+  bool ExperimentalAutoDetectBinPacking;
+
   /// \brief Allow putting all parameters of a function declaration onto
   /// the next line even if \c BinPackParameters is \c false.
   bool AllowAllParametersOfDeclarationOnNextLine;
@@ -155,6 +167,8 @@ struct FormatStyle {
            ConstructorInitializerAllOnOneLineOrOnePerLine ==
                R.ConstructorInitializerAllOnOneLineOrOnePerLine &&
            DerivePointerBinding == R.DerivePointerBinding &&
+           ExperimentalAutoDetectBinPacking ==
+               R.ExperimentalAutoDetectBinPacking &&
            IndentCaseLabels == R.IndentCaseLabels &&
            IndentWidth == R.IndentWidth &&
            MaxEmptyLinesToKeep == R.MaxEmptyLinesToKeep &&

diff --git a/lib/Format/Format.cpp b/lib/Format/Format.cpp
index b6b726cfb21558fa79046d060a73394ff9bb2fec..ad390c23dcd62efcf6b65a2a93bcb8c10d8c74a3 100644 (file)
--- a/lib/Format/Format.cpp
+++ b/lib/Format/Format.cpp
@@ -94,6 +94,8 @@ template <> struct MappingTraits<clang::format::FormatStyle> {
     IO.mapOptional("ConstructorInitializerAllOnOneLineOrOnePerLine",
                    Style.ConstructorInitializerAllOnOneLineOrOnePerLine);
     IO.mapOptional("DerivePointerBinding", Style.DerivePointerBinding);
+    IO.mapOptional("ExperimentalAutoDetectBinPacking",
+                   Style.ExperimentalAutoDetectBinPacking);
     IO.mapOptional("IndentCaseLabels", Style.IndentCaseLabels);
     IO.mapOptional("MaxEmptyLinesToKeep", Style.MaxEmptyLinesToKeep);
     IO.mapOptional("ObjCSpaceBeforeProtocolList",
@@ -134,6 +136,7 @@ FormatStyle getLLVMStyle() {
   LLVMStyle.ColumnLimit = 80;
   LLVMStyle.ConstructorInitializerAllOnOneLineOrOnePerLine = false;
   LLVMStyle.DerivePointerBinding = false;
+  LLVMStyle.ExperimentalAutoDetectBinPacking = false;
   LLVMStyle.IndentCaseLabels = false;
   LLVMStyle.MaxEmptyLinesToKeep = 1;
   LLVMStyle.ObjCSpaceBeforeProtocolList = true;
@@ -165,6 +168,7 @@ FormatStyle getGoogleStyle() {
   GoogleStyle.ColumnLimit = 80;
   GoogleStyle.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
   GoogleStyle.DerivePointerBinding = true;
+  GoogleStyle.ExperimentalAutoDetectBinPacking = false;
   GoogleStyle.IndentCaseLabels = true;
   GoogleStyle.MaxEmptyLinesToKeep = 1;
   GoogleStyle.ObjCSpaceBeforeProtocolList = false;
@@ -260,10 +264,12 @@ public:
                          const AnnotatedLine &Line, unsigned FirstIndent,
                          const FormatToken *RootToken,
                          WhitespaceManager &Whitespaces,
-                         encoding::Encoding Encoding)
+                         encoding::Encoding Encoding,
+                         bool BinPackInconclusiveFunctions)
       : Style(Style), SourceMgr(SourceMgr), Line(Line),
         FirstIndent(FirstIndent), RootToken(RootToken),
-        Whitespaces(Whitespaces), Count(0), Encoding(Encoding) {}
+        Whitespaces(Whitespaces), Count(0), Encoding(Encoding),
+        BinPackInconclusiveFunctions(BinPackInconclusiveFunctions) {}
 
   /// \brief Formats an \c UnwrappedLine.
   void format(const AnnotatedLine *NextLine) {
@@ -782,7 +788,11 @@ private:
       } else {
         NewIndent =
             4 + std::max(LastSpace, State.Stack.back().StartOfFunctionCall);
-        AvoidBinPacking = !Style.BinPackParameters;
+        AvoidBinPacking = !Style.BinPackParameters ||
+                          (Style.ExperimentalAutoDetectBinPacking &&
+                           (Current.PackingKind == PPK_OnePerLine ||
+                            (!BinPackInconclusiveFunctions &&
+                             Current.PackingKind == PPK_Inconclusive)));
       }
 
       State.Stack.push_back(ParenState(NewIndent, LastSpace, AvoidBinPacking,
@@ -1157,6 +1167,7 @@ private:
   // to create a deterministic order independent of the container.
   unsigned Count;
   encoding::Encoding Encoding;
+  bool BinPackInconclusiveFunctions;
 };
 
 class FormatTokenLexer {
@@ -1363,7 +1374,8 @@ public:
               1;
         }
         UnwrappedLineFormatter Formatter(Style, SourceMgr, TheLine, Indent,
-                                         TheLine.First, Whitespaces, Encoding);
+                                         TheLine.First, Whitespaces, Encoding,
+                                         BinPackInconclusiveFunctions);
         Formatter.format(I + 1 != E ? &*(I + 1) : NULL);
         IndentForLevel[TheLine.Level] = LevelIndent;
         PreviousLineWasTouched = true;
@@ -1408,6 +1420,8 @@ private:
     unsigned CountBoundToVariable = 0;
     unsigned CountBoundToType = 0;
     bool HasCpp03IncompatibleFormat = false;
+    bool HasBinPackedFunction = false;
+    bool HasOnePerLineFunction = false;
     for (unsigned i = 0, e = AnnotatedLines.size(); i != e; ++i) {
       if (!AnnotatedLines[i].First->Next)
         continue;
@@ -1428,6 +1442,12 @@ private:
             Tok->Previous->Type == TT_TemplateCloser &&
             Tok->WhitespaceRange.getBegin() == Tok->WhitespaceRange.getEnd())
           HasCpp03IncompatibleFormat = true;
+
+        if (Tok->PackingKind == PPK_BinPacked)
+          HasBinPackedFunction = true;
+        if (Tok->PackingKind == PPK_OnePerLine)
+          HasOnePerLineFunction = true;
+
         Tok = Tok->Next;
       }
     }
@@ -1441,6 +1461,8 @@ private:
       Style.Standard = HasCpp03IncompatibleFormat ? FormatStyle::LS_Cpp11
                                                   : FormatStyle::LS_Cpp03;
     }
+    BinPackInconclusiveFunctions =
+        HasBinPackedFunction || !HasOnePerLineFunction;
   }
 
   /// \brief Get the indent of \p Level from \p IndentForLevel.
@@ -1691,6 +1713,7 @@ private:
   std::vector<AnnotatedLine> AnnotatedLines;
 
   encoding::Encoding Encoding;
+  bool BinPackInconclusiveFunctions;
 };
 
 } // end anonymous namespace

diff --git a/lib/Format/FormatToken.h b/lib/Format/FormatToken.h
index 1de4b8964fe31726caa1c7e51fb9519c5b1650a9..03f097000f48716a782b423229eecfa14cd74f4a 100644 (file)
--- a/lib/Format/FormatToken.h
+++ b/lib/Format/FormatToken.h
@@ -64,6 +64,13 @@ enum BraceBlockKind {
   BK_BracedInit
 };
 
+// The packing kind of a function's parameters.
+enum ParameterPackingKind {
+  PPK_BinPacked,
+  PPK_OnePerLine,
+  PPK_Inconclusive
+};
+
 /// \brief A wrapper around a \c Token storing information about the
 /// whitespace characters preceeding it.
 struct FormatToken {
@@ -72,9 +79,9 @@ struct FormatToken {
         CodePointCount(0), IsFirst(false), MustBreakBefore(false),
         BlockKind(BK_Unknown), Type(TT_Unknown), SpacesRequiredBefore(0),
         CanBreakBefore(false), ClosesTemplateDeclaration(false),
-        ParameterCount(0), TotalLength(0), UnbreakableTailLength(0),
-        BindingStrength(0), SplitPenalty(0), LongestObjCSelectorName(0),
-        FakeRParens(0), LastInChainOfCalls(false),
+        ParameterCount(0), PackingKind(PPK_Inconclusive), TotalLength(0),
+        UnbreakableTailLength(0), BindingStrength(0), SplitPenalty(0),
+        LongestObjCSelectorName(0), FakeRParens(0), LastInChainOfCalls(false),
         PartOfMultiVariableDeclStmt(false), MatchingParen(NULL), Previous(NULL),
         Next(NULL) {}
 
@@ -143,6 +150,9 @@ struct FormatToken {
   /// the number of commas.
   unsigned ParameterCount;
 
+  /// \brief If this is an opening parenthesis, how are the parameters packed?
+  ParameterPackingKind PackingKind;
+
   /// \brief The total length of the line up to and including this token.
   unsigned TotalLength;
 

diff --git a/lib/Format/TokenAnnotator.cpp b/lib/Format/TokenAnnotator.cpp
index ce2db6d9c0bf7e6c724f6c1af4f42e5a97e9faf4..f5b6fbde7c9a3780ddd648272a72ab8913585ca3 100644 (file)
--- a/lib/Format/TokenAnnotator.cpp
+++ b/lib/Format/TokenAnnotator.cpp
@@ -99,6 +99,8 @@ private:
     }
 
     bool MightBeFunctionType = CurrentToken->is(tok::star);
+    bool HasMultipleLines = false;
+    bool HasMultipleParametersOnALine = false;
     while (CurrentToken != NULL) {
       // LookForDecls is set when "if (" has been seen. Check for
       // 'identifier' '*' 'identifier' followed by not '=' -- this
@@ -133,6 +135,13 @@ private:
           }
         }
 
+        if (!HasMultipleLines)
+          Left->PackingKind = PPK_Inconclusive;
+        else if (HasMultipleParametersOnALine)
+          Left->PackingKind = PPK_BinPacked;
+        else
+          Left->PackingKind = PPK_OnePerLine;
+
         next();
         return true;
       }
@@ -143,8 +152,14 @@ private:
                                                     tok::coloncolon))
         MightBeFunctionType = true;
       updateParameterCount(Left, CurrentToken);
+      if (CurrentToken->is(tok::comma) && CurrentToken->Next &&
+          !CurrentToken->Next->HasUnescapedNewline &&
+          !CurrentToken->Next->isTrailingComment())
+        HasMultipleParametersOnALine = true;
       if (!consumeToken())
         return false;
+      if (CurrentToken && CurrentToken->HasUnescapedNewline)
+        HasMultipleLines = true;
     }
     return false;
   }
@@ -245,10 +260,11 @@ private:
   }
 
   void updateParameterCount(FormatToken *Left, FormatToken *Current) {
-    if (Current->is(tok::comma))
+    if (Current->is(tok::comma)) {
       ++Left->ParameterCount;
-    else if (Left->ParameterCount == 0 && Current->isNot(tok::comment))
+    } else if (Left->ParameterCount == 0 && Current->isNot(tok::comment)) {
       Left->ParameterCount = 1;
+    }
   }
 
   bool parseConditional() {
@@ -1283,9 +1299,10 @@ void TokenAnnotator::printDebugInfo(const AnnotatedLine &Line) {
   const FormatToken *Tok = Line.First;
   while (Tok) {
     llvm::errs() << " M=" << Tok->MustBreakBefore
-                 << " C=" << Tok->CanBreakBefore << " T=" << Tok->Type << " S="
-                 << Tok->SpacesRequiredBefore << " P=" << Tok->SplitPenalty
-                 << " Name=" << Tok->Tok.getName() << " FakeLParens=";
+                 << " C=" << Tok->CanBreakBefore << " T=" << Tok->Type
+                 << " S=" << Tok->SpacesRequiredBefore
+                 << " P=" << Tok->SplitPenalty << " Name=" << Tok->Tok.getName()
+                 << " PPK=" << Tok->PackingKind << " FakeLParens=";
     for (unsigned i = 0, e = Tok->FakeLParens.size(); i != e; ++i)
       llvm::errs() << Tok->FakeLParens[i] << "/";
     llvm::errs() << " FakeRParens=" << Tok->FakeRParens << "\n";

diff --git a/unittests/Format/FormatTest.cpp b/unittests/Format/FormatTest.cpp
index 0192a0718b37a15ae5f7f26ecf22cfa11f924a45..b87393a9da277d5b3405fda9f30fbbda503b58d5 100644 (file)
--- a/unittests/Format/FormatTest.cpp
+++ b/unittests/Format/FormatTest.cpp
@@ -2644,6 +2644,30 @@ TEST_F(FormatTest, FormatsOneParameterPerLineIfNecessary) {
       NoBinPacking);
 }
 
+TEST_F(FormatTest, AdaptiveOnePerLineFormatting) {
+  FormatStyle Style = getLLVMStyleWithColumns(15);
+  Style.ExperimentalAutoDetectBinPacking = true;
+  EXPECT_EQ("aaa(aaaa,\n"
+            "    aaaa,\n"
+            "    aaaa);\n"
+            "aaa(aaaa,\n"
+            "    aaaa,\n"
+            "    aaaa);",
+            format("aaa(aaaa,\n" // one-per-line
+                   "  aaaa,\n"
+                   "    aaaa  );\n"
+                   "aaa(aaaa,  aaaa,  aaaa);", // inconclusive
+                   Style));
+  EXPECT_EQ("aaa(aaaa, aaaa,\n"
+            "    aaaa);\n"
+            "aaa(aaaa, aaaa,\n"
+            "    aaaa);",
+            format("aaa(aaaa,  aaaa,\n" // bin-packed
+                   "    aaaa  );\n"
+                   "aaa(aaaa,  aaaa,  aaaa);", // inconclusive
+                   Style));
+}
+
 TEST_F(FormatTest, FormatsBuilderPattern) {
   verifyFormat(
       "return llvm::StringSwitch<Reference::Kind>(name)\n"