[clang-format] New format param ObjCBinPackProtocolList

[benhamilton]

Summary

This is an alternative approach to D42014 after some
investigation by stephanemoore@ and myself.

Previously, the format parameter BinPackParameters controlled both
C function parameter list bin-packing and Objective-C protocol conformance
list bin-packing.

We found in the Google style, some teams were changing
BinPackParameters from its default (true) to false so they could
lay out Objective-C protocol conformance list items one-per-line
instead of bin-packing them into as few lines as possible.

To allow teams to use one-per-line Objective-C protocol lists without
changing bin-packing for other areas like C function parameter lists,
this diff introduces a new LibFormat parameter
ObjCBinPackProtocolList to control the behavior just for ObjC
protocol conformance lists.

The new parameter is an enum which defaults to Auto to keep the
previous behavior (delegating to BinPackParameters).

Depends On D42649

Test Plan: New tests added. make -j12 FormatTests && ./tools/clang/unittests/Format/FormatTests

[benhamilton]

Rebase

[stephanemoore]

lib/Format/ContinuationIndenter.cpp

1232

Given that we now have a new setting distinguishing bin packing of Objective-C protocol conformance lists and bin packing of parameters perhaps this local variable should be named more generally? Maybe "BinPackDeclaration" would be appropriate because this local variable takes effect if State.Line->MustBeDeclaration is true?

lib/Format/Format.cpp

765

I propose that we defer this to a future commit while we establish consensus at Google.

unittests/Format/FormatTestObjC.cpp

325–334

Similar to my other comment, I propose deferring the changes to Google style to a future commit.

[benhamilton]

Move Google style changes out. Use clearer name for local variable.

[benhamilton]

Thanks for the review!

lib/Format/Format.cpp

765

Moved to D42708.

unittests/Format/FormatTestObjC.cpp

325–334

Moved to D42708.

[benhamilton]

• BinPackObjCProtocolList -> ObjCBinPackProtocolList

[stephanemoore]

Looks good to me 👌 Thanks for driving this!

[benhamilton]

Thanks! @jolesiak is not available for the rest of the week, so should I just submit this as-is, or wait until next week?

[stephanemoore]

lib/Format/ContinuationIndenter.cpp

1214

With the new parameter does this comment still apply? Maybe we can remove it?

[benhamilton]

lib/Format/ContinuationIndenter.cpp

1214

Good point.

This is about <> brackets nested inside () parens, which I believe is applicable to Objective-C 2.0 generics passed to C/C++ functions, but not to protocol conformance lists.

I'll send out a separate diff to add tests and clean up this comment, since I think the behavior is wanted for ObjC as well, but I think it's unrelated to this diff.

[benhamilton]

lib/Format/ContinuationIndenter.cpp

1214

Followup in D42864.

[krasimir]

I don't understand why do we introduce an enum option if we are keeping the default behavior for Google style. IMO we should have a single behavior for any style and enforce it.

[benhamilton]

I don't understand why do we introduce an enum option if we are keeping the default behavior for Google style. IMO we should have a single behavior for any style and enforce it.

D42708 changes the default behavior for the Google style.

I'm fine with opening up a discussion to make this the default for all styles, but I think that should be a separate discussion.

include/clang/Format/Format.h

struct FormatStyle {
   ///
   ///   false:
   ///   void f(int aaaaaaaaaaaaaaaaaaaa,
   ///          int aaaaaaaaaaaaaaaaaaaa,
   ///          int aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {}
   /// \endcode
   bool BinPackParameters;
 
+  /// \brief The style of wrapping parameters on the same line (bin-packed) or
+  /// on one line each.
+  enum BinPackStyle {
+    /// Automatically determine parameter bin-packing behavior.
+    BPS_Auto,
+    /// Always bin-pack parameters.
+    BPS_Always,
+    /// Never bin-pack parameters.
+    BPS_Never,
+  };
+
+  /// \brief Controls bin-packing Objective-C protocol conformance list
+  /// items into as few lines as possible when they go over `ColumnLimit`.
+  ///
+  /// If ``Auto`` (the default), delegates to the value in
+  /// ``BinPackParameters``. If that is ``true``, bin-packs Objective-C
+  /// protocol conformance list items into as few lines as possible
+  /// whenever they go over ``ColumnLimit``.
+  ///
+  /// If ``Always``, always bin-packs Objective-C protocol conformance
+  /// list items into as few lines as possible whenever they go over
+  /// ``ColumnLimit``.
+  ///
+  /// If ``Never``, lays out Objective-C protocol conformance list items
+  /// onto individual lines whenever they go over ``ColumnLimit``.
+  ///
+  /// \code
+  ///    Always (or Auto, if BinPackParameters=true):
+  ///    @interface ccccccccccccc () <
+  ///        ccccccccccccc, ccccccccccccc,
+  ///        ccccccccccccc, ccccccccccccc> {
+  ///    }
+  ///
+  ///    Never (or Auto, if BinPackParameters=false):
+  ///    @interface ddddddddddddd () <
+  ///        ddddddddddddd,
+  ///        ddddddddddddd,
+  ///        ddddddddddddd,
+  ///        ddddddddddddd> {
+  ///    }
+  /// \endcode
+  BinPackStyle BinPackObjCProtocolList;
+
   /// \brief The style of breaking before or after binary operators.
   enum BinaryOperatorStyle {
     /// Break after operators.
     /// \code
     ///    LooooooooooongType loooooooooooooooooooooongVariable =
     ///        someLooooooooooooooooongFunction();
     ///
     ///    bool value = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +
return AccessModifierOffset == R.AccessModifierOffset &&
            AllowShortLoopsOnASingleLine == R.AllowShortLoopsOnASingleLine &&
            AlwaysBreakAfterReturnType == R.AlwaysBreakAfterReturnType &&
            AlwaysBreakBeforeMultilineStrings ==
                R.AlwaysBreakBeforeMultilineStrings &&
            AlwaysBreakTemplateDeclarations ==
                R.AlwaysBreakTemplateDeclarations &&
            BinPackArguments == R.BinPackArguments &&
            BinPackParameters == R.BinPackParameters &&
+           BinPackObjCProtocolList == R.BinPackObjCProtocolList &&
            BreakBeforeBinaryOperators == R.BreakBeforeBinaryOperators &&
            BreakBeforeBraces == R.BreakBeforeBraces &&
            BreakBeforeTernaryOperators == R.BreakBeforeTernaryOperators &&
            BreakConstructorInitializers == R.BreakConstructorInitializers &&
            CompactNamespaces == R.CompactNamespaces &&
            BreakAfterJavaFieldAnnotations == R.BreakAfterJavaFieldAnnotations &&
            BreakStringLiterals == R.BreakStringLiterals &&
            ColumnLimit == R.ColumnLimit && CommentPragmas == R.CommentPragmas &&

lib/Format/ContinuationIndenter.cpp

if (Current.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) ||
     NewIndent = Style.ContinuationIndentWidth +
                 std::max(State.Stack.back().LastSpace,
                          State.Stack.back().StartOfFunctionCall);
 
     // Ensure that different different brackets force relative alignment, e.g.:
     // void SomeFunction(vector<  // break
     //                       int> v);
     // FIXME: We likely want to do this for more combinations of brackets.
     // Verify that it is wanted for ObjC, too.
     if (Current.is(tok::less) && Current.ParentBracket == tok::l_paren) {
       NewIndent = std::max(NewIndent, State.Stack.back().Indent);
       LastSpace = std::max(LastSpace, State.Stack.back().Indent);
     }
 
     bool EndsInComma =
         Current.MatchingParen &&
         Current.MatchingParen->getPreviousNonComment() &&
         Current.MatchingParen->getPreviousNonComment()->is(tok::comma);
 
+    // If BinPackObjCProtocolList is unspecified, fall back to BinPackParameters
+    // for backwards compatibility.
+    bool BinPackObjCProtocolList =
+        (Style.BinPackObjCProtocolList == FormatStyle::BPS_Auto &&
+         Style.BinPackParameters) ||
+        Style.BinPackObjCProtocolList == FormatStyle::BPS_Always;
+
+    bool BinPackParameters =
+        (State.Line->Type != LT_ObjCDecl && Style.BinPackParameters) ||
+        (State.Line->Type == LT_ObjCDecl && BinPackObjCProtocolList);
+
     AvoidBinPacking =
         (Style.Language == FormatStyle::LK_JavaScript && EndsInComma) ||
-        (State.Line->MustBeDeclaration && !Style.BinPackParameters) ||
+        (State.Line->MustBeDeclaration && !BinPackParameters) ||
         (!State.Line->MustBeDeclaration && !Style.BinPackArguments) ||
         (Style.ExperimentalAutoDetectBinPacking &&
          (Current.PackingKind == PPK_OnePerLine ||
           (!BinPackInconclusiveFunctions &&
            Current.PackingKind == PPK_Inconclusive)));
 
     if (Current.is(TT_ObjCMethodExpr) && Current.MatchingParen) {
       if (Style.ColumnLimit) {

lib/Format/Format.cpp

static void enumeration(IO &IO, FormatStyle::ShortFunctionStyle &Value) {
     IO.enumCase(Value, "All", FormatStyle::SFS_All);
     IO.enumCase(Value, "true", FormatStyle::SFS_All);
     IO.enumCase(Value, "Inline", FormatStyle::SFS_Inline);
     IO.enumCase(Value, "InlineOnly", FormatStyle::SFS_InlineOnly);
     IO.enumCase(Value, "Empty", FormatStyle::SFS_Empty);
   }
 };
 
+template <> struct ScalarEnumerationTraits<FormatStyle::BinPackStyle> {
+  static void enumeration(IO &IO, FormatStyle::BinPackStyle &Value) {
+    IO.enumCase(Value, "Auto", FormatStyle::BPS_Auto);
+    IO.enumCase(Value, "Always", FormatStyle::BPS_Always);
+    IO.enumCase(Value, "Never", FormatStyle::BPS_Never);
+  }
+};
+
 template <> struct ScalarEnumerationTraits<FormatStyle::BinaryOperatorStyle> {
   static void enumeration(IO &IO, FormatStyle::BinaryOperatorStyle &Value) {
     IO.enumCase(Value, "All", FormatStyle::BOS_All);
     IO.enumCase(Value, "true", FormatStyle::BOS_All);
     IO.enumCase(Value, "None", FormatStyle::BOS_None);
     IO.enumCase(Value, "false", FormatStyle::BOS_None);
     IO.enumCase(Value, "NonAssignment", FormatStyle::BOS_NonAssignment);
   }
static void mapping(IO &IO, FormatStyle &Style) {
     }
 
     IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
                    Style.AlwaysBreakBeforeMultilineStrings);
     IO.mapOptional("AlwaysBreakTemplateDeclarations",
                    Style.AlwaysBreakTemplateDeclarations);
     IO.mapOptional("BinPackArguments", Style.BinPackArguments);
     IO.mapOptional("BinPackParameters", Style.BinPackParameters);
+    IO.mapOptional("BinPackObjCProtocolList", Style.BinPackObjCProtocolList);
     IO.mapOptional("BraceWrapping", Style.BraceWrapping);
     IO.mapOptional("BreakBeforeBinaryOperators",
                    Style.BreakBeforeBinaryOperators);
     IO.mapOptional("BreakBeforeBraces", Style.BreakBeforeBraces);
     IO.mapOptional("BreakBeforeInheritanceComma",
                    Style.BreakBeforeInheritanceComma);
     IO.mapOptional("BreakBeforeTernaryOperators",
                    Style.BreakBeforeTernaryOperators);
FormatStyle getLLVMStyle() {
   LLVMStyle.AllowShortIfStatementsOnASingleLine = false;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AlwaysBreakTemplateDeclarations = false;
   LLVMStyle.BinPackArguments = true;
   LLVMStyle.BinPackParameters = true;
+  LLVMStyle.BinPackObjCProtocolList = FormatStyle::BPS_Auto;
   LLVMStyle.BreakBeforeBinaryOperators = FormatStyle::BOS_None;
   LLVMStyle.BreakBeforeTernaryOperators = true;
   LLVMStyle.BreakBeforeBraces = FormatStyle::BS_Attach;
   LLVMStyle.BraceWrapping = {false, false, false, false, false,
                              false, false, false, false, false,
                              false, false, true,  true,  true};
   LLVMStyle.BreakAfterJavaFieldAnnotations = false;
   LLVMStyle.BreakConstructorInitializers = FormatStyle::BCIS_BeforeColon;
if (Language == FormatStyle::LK_Java) {
     GoogleStyle.SpacesInContainerLiterals = false;
     GoogleStyle.JavaScriptQuotes = FormatStyle::JSQS_Single;
     GoogleStyle.JavaScriptWrapImports = false;
   } else if (Language == FormatStyle::LK_Proto) {
     GoogleStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
     GoogleStyle.SpacesInContainerLiterals = false;
   } else if (Language == FormatStyle::LK_ObjC) {
     GoogleStyle.ColumnLimit = 100;
+    GoogleStyle.BinPackObjCProtocolList = FormatStyle::BPS_Never;
   }
 
   return GoogleStyle;
 }
 
 FormatStyle getChromiumStyle(FormatStyle::LanguageKind Language) {
   FormatStyle ChromiumStyle = getGoogleStyle(Language);
   if (Language == FormatStyle::LK_Java) {

unittests/Format/FormatTestObjC.cpp

verifyFormat("@interface Foo (HackStuff) <MyProtocol> {\n"
                "+ (id)init;\n"
                "@end");
 
   Style.ColumnLimit = 40;
   verifyFormat("@interface ccccccccccccc () <\n"
                "    ccccccccccccc, ccccccccccccc,\n"
                "    ccccccccccccc, ccccccccccccc> {\n"
                "}");
-
-  Style.BinPackParameters = false;
+  Style.BinPackObjCProtocolList = FormatStyle::BPS_Never;
   verifyFormat("@interface ddddddddddddd () <\n"
                "    ddddddddddddd,\n"
                "    ddddddddddddd,\n"
                "    ddddddddddddd,\n"
                "    ddddddddddddd> {\n"
                "}");
 
+  Style.BinPackParameters = false;
+  Style.BinPackObjCProtocolList = FormatStyle::BPS_Auto;
+  verifyFormat("@interface eeeeeeeeeeeee () <\n"
+               "    eeeeeeeeeeeee,\n"
+               "    eeeeeeeeeeeee,\n"
+               "    eeeeeeeeeeeee,\n"
+               "    eeeeeeeeeeeee> {\n"
+               "}");
+  Style.BinPackObjCProtocolList = FormatStyle::BPS_Always;
+  verifyFormat("@interface fffffffffffff () <\n"
+               "    fffffffffffff, fffffffffffff,\n"
+               "    fffffffffffff, fffffffffffff> {\n"
+               "}");
+
   Style = getGoogleStyle(FormatStyle::LK_ObjC);
   verifyFormat("@interface Foo : NSObject <NSSomeDelegate> {\n"
                " @public\n"
                "  int field1;\n"
                " @protected\n"
                "  int field2;\n"
                " @private\n"
                "  int field3;\n"
                " @package\n"
                "  int field4;\n"
                "}\n"
                "+ (id)init;\n"
                "@end");
   verifyFormat("@interface Foo : Bar <Baz, Quux>\n"
                "+ (id)init;\n"
                "@end");
   verifyFormat("@interface Foo (HackStuff) <MyProtocol>\n"
                "+ (id)init;\n"
                "@end");
-  Style.BinPackParameters = false;
-  Style.ColumnLimit = 80;
-  verifyFormat("@interface aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa () <\n"
-               "    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,\n"
-               "    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,\n"
-               "    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,\n"
-               "    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa> {\n"
+  Style.ColumnLimit = 40;
+  // BinPackParameters should be true by default.
+  verifyFormat("void eeeeeeee(int eeeee, int eeeee,\n"
+               "              int eeeee, int eeeee);\n");
+  // BinPackObjCProtocolList should be BPS_Never by default.
+  verifyFormat("@interface fffffffffffff () <\n"
+               "    fffffffffffff,\n"
+               "    fffffffffffff,\n"
+               "    fffffffffffff,\n"
+               "    fffffffffffff> {\n"
                "}");
 }
 
 TEST_F(FormatTestObjC, FormatObjCImplementation) {
   verifyFormat("@implementation Foo : NSObject {\n"
                "@public\n"
                "  int field1;\n"
                "@protected\n"