[clang-format] Add BinPackLongBracedList style option

[gedare]

The use of Cpp11BracedListStyle with BinPackArguments=False avoids bin packing until reaching a hard-coded limit of 20 items. This is an arbitrary choice. Introduce a new style option to allow disabling this limit.

[gedare]

For the history, see #20997

[llvmbot]

@llvm/pr-subscribers-clang-format

@llvm/pr-subscribers-clang

Author: Gedare Bloom (gedare)

Changes

The use of Cpp11BracedListStyle with BinPackParameters=False avoids bin packing until reaching a hard-coded limit of 20 items. This is an arbitrary choice. Introduce a new style option to allow setting a configurable limit of items.

---

Full diff: https://github.com/llvm/llvm-project/pull/112482.diff

6 Files Affected:

• (modified) clang/docs/ClangFormatStyleOptions.rst (+20)
• (modified) clang/docs/ReleaseNotes.rst (+2)
• (modified) clang/include/clang/Format/Format.h (+19)
• (modified) clang/lib/Format/Format.cpp (+3)
• (modified) clang/lib/Format/FormatToken.cpp (+1-1)
• (modified) clang/unittests/Format/FormatTest.cpp (+26)

diff --git a/clang/docs/ClangFormatStyleOptions.rst b/clang/docs/ClangFormatStyleOptions.rst
index 8add0a53e5be13..d0440df388aeb0 100644
--- a/clang/docs/ClangFormatStyleOptions.rst
+++ b/clang/docs/ClangFormatStyleOptions.rst
@@ -3769,6 +3769,9 @@ the configuration (without a prefix: ``Auto``).
   the parentheses of a function call with that name. If there is no name,
   a zero-length name is assumed.
 
+  ``BinPackArguments`` may be ignored for initializer lists with more than
+  ``MaxSingleLinesInBracedList`` items.
+
   .. code-block:: c++
 
      true:                                  false:
@@ -4883,6 +4886,23 @@ the configuration (without a prefix: ``Auto``).
        return i;
      }
 
+.. _MaxSingleLinesInBracedList:
+
+**MaxSingleLinesInBracedList** (``Unsigned``) :versionbadge:`clang-format 20` :ref:`¶ <MaxSingleLinesInBracedList>`
+  The maximum number of single item lines to keep in a braced list when
+  ``BinPackArguments`` is ``false`` before formatting items in columns.
+  Defaults to 20.
+
+  .. code-block:: c++
+
+     MaxSingleLinesInBracedList: 5  vs.     MaxSingleLinesInBracedList: 4
+     vector<int> x{                         vector<int> x{1, 2, 3, 4, 5};
+                 1,
+                 2,
+                 3,
+                 4,
+                 5};
+
 .. _NamespaceIndentation:
 
 **NamespaceIndentation** (``NamespaceIndentationKind``) :versionbadge:`clang-format 3.7` :ref:`¶ <NamespaceIndentation>`
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 817e3abef8d566..9807d8c7bc6362 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -674,6 +674,8 @@ clang-format
 - Adds ``AlignFunctionDeclarations`` option to ``AlignConsecutiveDeclarations``.
 - Adds ``IndentOnly`` suboption to ``ReflowComments`` to fix the indentation of multi-line comments
   without touching their contents, renames ``false`` to ``Never``, and ``true`` to ``Always``.
+- Adds ``MaxSingleLinesInBracedList`` option to control the limit on how many 
+  single lines to use when avoiding bin packing with braced list initializers.
 
 libclang
 --------
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index a0762b088b68ef..19a301dbc66403 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -2515,6 +2515,9 @@ struct FormatStyle {
   /// (e.g. a type or variable name), clang-format formats as if the ``{}`` were
   /// the parentheses of a function call with that name. If there is no name,
   /// a zero-length name is assumed.
+  ///
+  /// ``BinPackArguments`` may be ignored for initializer lists with more than
+  /// ``MaxSingleLinesInBracedList`` items.
   /// \code
   ///    true:                                  false:
   ///    vector<int> x{1, 2, 3, 4};     vs.     vector<int> x{ 1, 2, 3, 4 };
@@ -3391,6 +3394,21 @@ struct FormatStyle {
   /// \version 3.7
   unsigned MaxEmptyLinesToKeep;
 
+  /// The maximum number of single item lines to keep in a braced list when
+  /// ``BinPackArguments`` is ``false`` before formatting items in columns.
+  /// Defaults to 20.
+  /// \code
+  ///    MaxSingleLinesInBracedList: 5  vs.     MaxSingleLinesInBracedList: 4
+  ///    vector<int> x{                         vector<int> x{1, 2, 3, 4, 5};
+  ///                1,
+  ///                2,
+  ///                3,
+  ///                4,
+  ///                5};
+  /// \endcode
+  /// \version 20
+  unsigned MaxSingleLinesInBracedList;
+
   /// Different ways to indent namespace contents.
   enum NamespaceIndentationKind : int8_t {
     /// Don't indent in namespaces.
@@ -5204,6 +5222,7 @@ struct FormatStyle {
            LineEnding == R.LineEnding && MacroBlockBegin == R.MacroBlockBegin &&
            MacroBlockEnd == R.MacroBlockEnd && Macros == R.Macros &&
            MaxEmptyLinesToKeep == R.MaxEmptyLinesToKeep &&
+           MaxSingleLinesInBracedList == R.MaxSingleLinesInBracedList &&
            NamespaceIndentation == R.NamespaceIndentation &&
            NamespaceMacros == R.NamespaceMacros &&
            ObjCBinPackProtocolList == R.ObjCBinPackProtocolList &&
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index 148270795c562f..d5d74591fb593a 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -1059,6 +1059,8 @@ template <> struct MappingTraits<FormatStyle> {
     IO.mapOptional("Macros", Style.Macros);
     IO.mapOptional("MainIncludeChar", Style.IncludeStyle.MainIncludeChar);
     IO.mapOptional("MaxEmptyLinesToKeep", Style.MaxEmptyLinesToKeep);
+    IO.mapOptional("MaxSingleLinesInBracedList",
+                   Style.MaxSingleLinesInBracedList);
     IO.mapOptional("NamespaceIndentation", Style.NamespaceIndentation);
     IO.mapOptional("NamespaceMacros", Style.NamespaceMacros);
     IO.mapOptional("ObjCBinPackProtocolList", Style.ObjCBinPackProtocolList);
@@ -1569,6 +1571,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.Language = Language;
   LLVMStyle.LineEnding = FormatStyle::LE_DeriveLF;
   LLVMStyle.MaxEmptyLinesToKeep = 1;
+  LLVMStyle.MaxSingleLinesInBracedList = 20;
   LLVMStyle.NamespaceIndentation = FormatStyle::NI_None;
   LLVMStyle.ObjCBinPackProtocolList = FormatStyle::BPS_Auto;
   LLVMStyle.ObjCBlockIndentWidth = 2;
diff --git a/clang/lib/Format/FormatToken.cpp b/clang/lib/Format/FormatToken.cpp
index 963e8f87793fa0..31327feb4d286f 100644
--- a/clang/lib/Format/FormatToken.cpp
+++ b/clang/lib/Format/FormatToken.cpp
@@ -174,7 +174,7 @@ void CommaSeparatedList::precomputeFormattingInfos(const FormatToken *Token) {
   // have many items (20 or more) or we allow bin-packing of function call
   // arguments.
   if (Style.Cpp11BracedListStyle && !Style.BinPackArguments &&
-      Commas.size() < 19) {
+      Commas.size() <= Style.MaxSingleLinesInBracedList) {
     return;
   }
 
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 43513f18321bc0..a97aea878f5578 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -14038,6 +14038,32 @@ TEST_F(FormatTest, LayoutCxx11BraceInitializers) {
       "    ffffff, ggggg,  hhhhhh, iiiiii, jjjjjj, kkkkkk,\n"
       "};",
       NoBinPacking);
+  NoBinPacking.MaxSingleLinesInBracedList = 22;
+  verifyFormat("const Aaaaaa aaaaa = {\n"
+               "    aaaaa,\n"
+               "    bbbbb,\n"
+               "    ccccc,\n"
+               "    ddddd,\n"
+               "    eeeee,\n"
+               "    ffffff,\n"
+               "    ggggg,\n"
+               "    hhhhhh,\n"
+               "    iiiiii,\n"
+               "    jjjjjj,\n"
+               "    kkkkkk,\n"
+               "    aaaaa,\n"
+               "    bbbbb,\n"
+               "    ccccc,\n"
+               "    ddddd,\n"
+               "    eeeee,\n"
+               "    ffffff,\n"
+               "    ggggg,\n"
+               "    hhhhhh,\n"
+               "    iiiiii,\n"
+               "    jjjjjj,\n"
+               "    kkkkkk,\n"
+               "};",
+               NoBinPacking);
 
   NoBinPacking.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
   verifyFormat("static uint8 CddDp83848Reg[] = {\n"

[gedare]

I would also be ok to have a style option to make the existing 20-item limit optional (but enabled by default). That might be a little more maintainable than allowing arbitrary limits to be used. There will be bugs if/when people use this option with small values for the limit, as it will interact weirdly with both the braced list initializer formatting rules, and also with the AvoidBinPacking logic of the continuation indenter.

[owenca]

There will be bugs if/when people use this option with small values for the limit, as it will interact weirdly with both the braced list initializer formatting rules, and also with the AvoidBinPacking logic of the continuation indenter.

Can you give some examples?

[gedare]

There will be bugs if/when people use this option with small values for the limit, as it will interact weirdly with both the braced list initializer formatting rules, and also with the AvoidBinPacking logic of the continuation indenter.

Can you give some examples?

echo "vector<int> x{1111, 2, 3, 4, 5};" > a.txt
echo "vector<int> x{1111, 2, 3, 4, 5, 6};" > b.txt

LLVM style, actual output OK:

clang-format --style="{BasedOnStyle: llvm, ColumnLimit: 20}" a.txt

vector<int> x{1111,
              2, 3,
              4, 5};

clang-format --style="{BasedOnStyle: llvm, ColumnLimit: 20}" b.txt

vector<int> x{
    1111, 2, 3,
    4,    5, 6};

LLVM with braced list and no binpack, actual output OK:

clang-format --style="{BasedOnStyle: llvm, ColumnLimit: 20, BinPackArguments: false, Cpp11BracedListStyle: true}" a.txt

vector<int> x{1111,
              2,
              3,
              4,
              5};

clang-format --style="{BasedOnStyle: llvm, ColumnLimit: 20, BinPackArguments: false, Cpp11BracedListStyle: true}" b.txt

vector<int> x{1111,
              2,
              3,
              4,
              5,
              6};

Same, but limit single entry lines to 3, Actual output not OK for a.txt, OK for b.txt

clang-format --style="{BasedOnStyle: llvm, ColumnLimit: 20, BinPackArguments: false, Cpp11BracedListStyle: true, MaxSingleLinesInBracedList: 3}" a.txt

vector<int> x{1111,
              2,
              3,
              4,
              5};

clang-format --style="{BasedOnStyle: llvm, ColumnLimit: 20, BinPackArguments: false, Cpp11BracedListStyle: true, MaxSingleLinesInBracedList: 3}" b.txt

vector<int> x{
    1111, 2, 3,
    4,    5, 6};

The expected output for a.txt should be something like:

vector<int> x{
    1111, 2, 3,
    4,    5};

So the maximum limit is not enforced in certain cases. I only see this with small values of MaxSingleLinesInBracedList (below 5 or so).

I would be just as happy with making the current hard-coded limit of 20 be optional to disable. This limit doesn't seem to bother many people.

[gedare]

So it turns out that this buggy behavior I'm seeing is also present in unmodified clang-format when the first item in the list is longer than the rest of the items. For example:

echo "vector<int> x{1111111111111, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24};" > c.txt
clang-format --style=llvm c.txt

vector<int> x{1111111111111,
              2,
              3,
              4,
              5,
              6,
              7,
              8,
              9,
              10,
              11,
              12,
              13,
              14,
              15,
              16,
              17,
              18,
              19,
              20,
              21,
              22,
              23,
              24};

versus:

echo "vector<int> x{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24};" > d.txt
clang-format --style=llvm d.txt
vector<int> x{1,  2,  3,  4,  5,  6,  7,  8,  9,  10, 11, 12,
              13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24};

[gedare]

I decided to go the simpler route and create a boolean option that can be used to disable the hard-coded limit of 20 items.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[gedare]

I'm content with this solution.

[HazardyKnusperkeks]

I think it should be merges with BinPackArguments to get an enum

• Never
• TwentyOrAbove (name is debatable)
• Always
• (Leave?)

[gedare]

I think it should be merges with BinPackArguments to get an enum

• Never
• TwentyOrAbove (name is debatable)
• Always
• (Leave?)

Currently the 20 item limit is only enforced on C++ initializer lists. Making it part of the BinPackArguments means it has to apply to any bin packing situation, so the scope of the style option increases. I'm not convinced it is worth doing.

[HazardyKnusperkeks]

I think it should be merges with BinPackArguments to get an enum

• Never
• TwentyOrAbove (name is debatable)
• Always
• (Leave?)

Currently the 20 item limit is only enforced on C++ initializer lists. Making it part of the BinPackArguments means it has to apply to any bin packing situation, so the scope of the style option increases. I'm not convinced it is worth doing.

Fair enough.

[gedare]

@owenca if you could take another look at this PR I'd appreciate it.

[owenca]

What about changing it to an option (e.g. BinPackBracedListThreshold) with a default value of about 20? If the number of elements in the list is more than the threshold, they will be bin packed even if BinPackArguments is set to false.

[gedare]

What about changing it to an option (e.g. BinPackBracedListThreshold) with a default value of about 20? If the number of elements in the list is more than the threshold, they will be bin packed even if BinPackArguments is set to false.

That was my original approach, but it is (marginally) more complex without a lot of obvious benefit to me, and it doesn't work well for small values, see #112482 (comment)

Since I did not find an open Issue about this topic, I leaned toward the less complex way of just allowing to enable/disable the currently hard-coded limit. I can make it a threshold instead, but I don't really want to find and fix the bugs that reveals.

[gedare]

Review comments addressed. Nice splitting of the test case.