[clang-format] Support block indenting array/struct list initializers

[gedare]

Summary

C89 and C99 list initializers are treated differently than Cpp11 braced initializers. This patch identifies the C array/struct initializer lists by finding the preceding equal sign before a left brace, and applies formatting rules for BracketAlignmentStyle.BlockIndent to those list initializers.

Addresses Issue #57878. https://github.com/llvm/llvm-project/issues/57878

[owenpan]

It seems to me that there has been a proliferation of new options being proposed and/or accepted recently. I'd like to remind everyone of the long-standing policy of adding new options. That being said, I wonder if we should add a new language LK_C along with its variants C78 (i.e. K&R C), C89 (i.e. ANSI C), GNU extensions (e.g. #55745 and #62755), etc.

[gedare]

In D153205#4430528, @owenpan wrote:

It seems to me that there has been a proliferation of new options being proposed and/or accepted recently. I'd like to remind everyone of the long-standing policy of adding new options. That being said, I wonder if we should add a new language LK_C along with its variants C78 (i.e. K&R C), C89 (i.e. ANSI C), GNU extensions (e.g. #55745 and #62755), etc.

Understood. In this case, the BK_ListInit is not visible at the API / Style level. It is an internal categorization to help distinguish different kinds of blocks.

[HazardyKnusperkeks]

In D153205#4430528, @owenpan wrote:

It seems to me that there has been a proliferation of new options being proposed and/or accepted recently. I'd like to remind everyone of the long-standing policy of adding new options. That being said, I wonder if we should add a new language LK_C along with its variants C78 (i.e. K&R C), C89 (i.e. ANSI C), GNU extensions (e.g. #55745 and #62755), etc.

That was already discussed: D117416#3250415

You add a lot of checks and I honestly can't say if it does not affect other code that is not covered by the tests.

clang/lib/Format/FormatToken.cpp

79

85–87

Which clang-format should've added.

[MyDeveloperDay]

clang/lib/Format/FormatToken.cpp

79

why pull this out? if you are worried about speed now you do it EVERY time whereas before we wouldn't do it unless limited conditions, for me I'd get rid of bk and just use getBlockKind() directly everytime, the compiler should I hope optimise it away

clang/unittests/Format/FormatTest.cpp

25625

should there be a semi colon here?

25642

should there be a semi colon here?

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

25511

nit [2]

[MyDeveloperDay]

In principle I like this as its annoyed me for years

[gedare]

In D153205#4431235, @HazardyKnusperkeks wrote:

You add a lot of checks and I honestly can't say if it does not affect other code that is not covered by the tests.

Not much I can do about that. I guess it is worth noting that all the checks I add are predicated on either BK_ListInit or BK_BracedInit. I have reduced the checks to what minimally works to address the Issue. Technically, this has two distinct changes included:

• Allow BlockIndent to work with Braces.
• Allow BlockIndent to work with array/struct initializers.

[gedare]

Do not use temporary variable for getBlockKind().
Fix test case syntax errors.
Fix formatting.

[MyDeveloperDay]

My only concern is that this changes behaviour without someone making the conscious decision to make that change, now as much as I can't understand why someone would want to put the } onto the same line, it will only take on person to say "hey I liked it like that" to complain and call it a regression, or that we somehow change the defaults (which people always complain about even through we don't normally do that)

In the past our solution to this problem is to have an option with a "Leave" setting to sort of say, leave it as it currently is. In this case it feels like a bug, looks like a bug and is probably a bug, but I'd be interested in @owenpan and @HazardyKnusperkeks's view

[HazardyKnusperkeks]

In D153205#4437966, @MyDeveloperDay wrote:

My only concern is that this changes behaviour without someone making the conscious decision to make that change, now as much as I can't understand why someone would want to put the } onto the same line, it will only take on person to say "hey I liked it like that" to complain and call it a regression, or that we somehow change the defaults (which people always complain about even through we don't normally do that)

In the past our solution to this problem is to have an option with a "Leave" setting to sort of say, leave it as it currently is. In this case it feels like a bug, looks like a bug and is probably a bug, but I'd be interested in @owenpan and @HazardyKnusperkeks's view

The github issue basically says all there's to say, if it those constructs should be formatted like function calls, then that's what should happen. Or we should adapt the documentation.

[gedare]

Can someone land for me? thanks.

[owenpan]

In D153205#4438782, @HazardyKnusperkeks wrote:

In D153205#4437966, @MyDeveloperDay wrote:

My only concern is that this changes behaviour without someone making the conscious decision to make that change, now as much as I can't understand why someone would want to put the } onto the same line, it will only take on person to say "hey I liked it like that" to complain and call it a regression, or that we somehow change the defaults (which people always complain about even through we don't normally do that)

In the past our solution to this problem is to have an option with a "Leave" setting to sort of say, leave it as it currently is. In this case it feels like a bug, looks like a bug and is probably a bug, but I'd be interested in @owenpan and @HazardyKnusperkeks's view

The github issue basically says all there's to say, if it those constructs should be formatted like function calls, then that's what should happen. Or we should adapt the documentation.

There is a warning in the document. (See here.) It should be updated in this patch.

I understand the concern raised by @MyDeveloperDay, but given that BAS_AlwaysBreak already covers the style of breaking after the opening brace but not before the closing brace, I'm okay with this change.

clang/lib/Format/ContinuationIndenter.cpp

365–367

And isBlockIndentedInitRBrace() returns true only if the matching l_brace is of BK_BracedInit or preceded by an =.

1175

All unit tests still pass.

clang/lib/Format/FormatToken.cpp

84–89

This seems redundant and can be deleted.

clang/lib/Format/FormatToken.h

185

We don't need to add a new block kind. Instead, checking if the previous token is = should work.

clang/lib/Format/TokenAnnotator.cpp

859

Either BK_ListInit is superfluous here or a unit test is missing.

4206

Ditto.

5448–5450

clang/unittests/Format/FormatTest.cpp

5042

Ditto below.

[gedare]

Address comments from owenpan.

Do not add the new block type, and instead check explicitly for the equal sign
before an opening right brace.

NOTE: Clang-Format Team Automated Review Comment

Your review contains a change to ClangFormatStyleOptions.rst but not a change to clang/include/clang/Format/Format.h

ClangFormatStyleOptions.rst is auto generated from Format.h via clang/docs/tools/dump_format_style.py, please run this to regenerate the .rst

You can validate that the rst is valid by running.

./docs/tools/dump_format_style.py
mkdir -p html
/usr/bin/sphinx-build -n ./docs ./html

[gedare]

Regenerate docs

[gedare]

clang/lib/Format/ContinuationIndenter.cpp

365–367

yes, this works nicely. thanks for spotting it.

[owenpan]

clang/include/clang/Format/Format.h

95–98 ↗

(On Diff #537402)

clang/lib/Format/FormatToken.cpp

79–81

85–86

clang/unittests/Format/FormatTest.cpp

25519

Ditto below.

[gedare]

clang/include/clang/Format/Format.h

95–98 ↗

(On Diff #537402)

This \note is not currently supported? I have submitted a rev D154552 to add such ability.

[gedare]

Address comments

[owenpan]

Do you need to update the current diff to show the net change? For example, the unit tests you added don't show up as a diff right now.

clang/include/clang/Format/Format.h

95–98 ↗

(On Diff #537402)

Ah, you are right. Please still use warning here and update D154552 after this patch lands.

[gedare]

Update and refresh diff

[gedare]

In D153205#4475836, @owenpan wrote:

Do you need to update the current diff to show the net change? For example, the unit tests you added don't show up as a diff right now.

Done. Not sure what happened there. But it looks right now.

clang/lib/Format/ContinuationIndenter.cpp

bool ContinuationIndenter::mustBreak(const LineState &State) {
   }
   if (Current.MustBreakBefore ||
       (Current.is(TT_InlineASMColon) &&
        (Style.BreakBeforeInlineASMColon == FormatStyle::BBIAS_Always ||
         Style.BreakBeforeInlineASMColon == FormatStyle::BBIAS_OnlyMultiline))) {
     return true;
   }
   if (CurrentState.BreakBeforeClosingBrace &&
-      Current.closesBlockOrBlockTypeList(Style)) {
+      (Current.closesBlockOrBlockTypeList(Style) ||
+       (Current.is(tok::r_brace) && Style.Cpp11BracedListStyle &&
+        Current.MatchingParen->isOneOf(BK_BracedInit, BK_ListInit) &&
+        Style.AlignAfterOpenBracket == FormatStyle::BAS_BlockIndent))) {
     return true;
   }
   if (CurrentState.BreakBeforeClosingParen && Current.is(tok::r_paren))
     return true;
   if (Style.Language == FormatStyle::LK_ObjC &&
       Style.ObjCBreakBeforeNestedBlockParam &&
       Current.ObjCSelectorNameParts > 1 &&
       Current.startsSequence(TT_SelectorName, tok::colon, tok::caret)) {
unsigned ContinuationIndenter::getNewLineColumn(const LineState &State) {
   //       code(); //
   //     }
   if (Current.is(tok::r_paren) && State.Stack.size() > 1 &&
       (!Current.Next ||
        Current.Next->isOneOf(tok::semi, tok::kw_const, tok::l_brace))) {
     return State.Stack[State.Stack.size() - 2].LastSpace;
   }
   if (Style.AlignAfterOpenBracket == FormatStyle::BAS_BlockIndent &&
-      Current.is(tok::r_paren) && State.Stack.size() > 1) {
+      (Current.is(tok::r_paren) ||
+       (Current.is(tok::r_brace) &&
+        Current.MatchingParen->isOneOf(BK_BracedInit, BK_ListInit))) &&
+      State.Stack.size() > 1) {
     return State.Stack[State.Stack.size() - 2].LastSpace;
   }
   if (NextNonComment->is(TT_TemplateString) && NextNonComment->closesScope())
     return State.Stack[State.Stack.size() - 2].LastSpace;
   if (Current.is(tok::identifier) && Current.Next &&
       (Current.Next->is(TT_DictLiteral) ||
        ((Style.Language == FormatStyle::LK_Proto ||
          Style.Language == FormatStyle::LK_TextProto) &&

clang/lib/Format/FormatToken.h

 #undef TYPE
       NUM_TOKEN_TYPES
 };
 
 /// Determines the name of a token type.
 const char *getTokenTypeName(TokenType Type);
 
 // Represents what type of block a set of braces open.
-enum BraceBlockKind { BK_Unknown, BK_Block, BK_BracedInit };
+enum BraceBlockKind { BK_Unknown, BK_Block, BK_BracedInit, BK_ListInit };
 
 // The packing kind of a function's parameters.
 enum ParameterPackingKind { PPK_BinPacked, PPK_OnePerLine, PPK_Inconclusive };
 
 enum FormatDecision { FD_Unformatted, FD_Continue, FD_Break };
 
 /// Roles a token can take in a configured macro expansion.
 enum MacroRole {

clang/lib/Format/FormatToken.cpp

#include "clang/Basic/TransformTypeTraits.def"
   }
 }
 
 bool FormatToken::isTypeOrIdentifier() const {
   return isSimpleTypeSpecifier() || Tok.isOneOf(tok::kw_auto, tok::identifier);
 }
 
 bool FormatToken::opensBlockOrBlockTypeList(const FormatStyle &Style) const {
   // C# Does not indent object initialisers as continuations.
   if (is(tok::l_brace) && getBlockKind() == BK_BracedInit && Style.isCSharp())
     return true;
   if (is(TT_TemplateString) && opensScope())
     return true;
+  // Indent C/C++ initializer lists as continuations.
+  if (is(tok::l_brace) &&
+      (getBlockKind() == BK_BracedInit || getBlockKind() == BK_ListInit) &&
+      Style.Cpp11BracedListStyle && Style.Language == FormatStyle::LK_Cpp) {
+    return false;
+  }
   return is(TT_ArrayInitializerLSquare) || is(TT_ProtoExtensionLSquare) ||
          (is(tok::l_brace) &&
           (getBlockKind() == BK_Block || is(TT_DictLiteral) ||
            (!Style.Cpp11BracedListStyle && NestingLevel == 0))) ||
          (is(tok::less) && (Style.Language == FormatStyle::LK_Proto ||
                             Style.Language == FormatStyle::LK_TextProto));
 }
 

clang/lib/Format/TokenAnnotator.cpp

bool parseBrace() {
     OpeningBrace.ParentBracket = Contexts.back().ContextKind;
 
     if (Contexts.back().CaretFound)
       OpeningBrace.overwriteFixedType(TT_ObjCBlockLBrace);
     Contexts.back().CaretFound = false;
 
     ScopedContextCreator ContextCreator(*this, tok::l_brace, 1);
     Contexts.back().ColonIsDictLiteral = true;
-    if (OpeningBrace.is(BK_BracedInit))
+    if (OpeningBrace.isOneOf(BK_BracedInit, BK_ListInit))
       Contexts.back().IsExpression = true;
     if (Style.isJavaScript() && OpeningBrace.Previous &&
         OpeningBrace.Previous->is(TT_JsTypeColon)) {
       Contexts.back().IsExpression = false;
     }
 
     unsigned CommaCount = 0;
     while (CurrentToken) {
if (Left.is(tok::ellipsis) && Right.is(tok::identifier) &&
       return false;
     }
     // Space in __attribute__((attr)) ::type.
     if (Left.is(TT_AttributeParen) && Right.is(tok::coloncolon))
       return true;
 
     if (Left.is(tok::kw_operator))
       return Right.is(tok::coloncolon);
-    if (Right.is(tok::l_brace) && Right.is(BK_BracedInit) &&
+    if (Right.is(tok::l_brace) && Right.isOneOf(BK_BracedInit, BK_ListInit) &&
         !Left.opensScope() && Style.SpaceBeforeCpp11BracedList) {
       return true;
     }
     if (Left.is(tok::less) && Left.is(TT_OverloadedOperator) &&
         Right.is(TT_TemplateOpener)) {
       return true;
     }
   } else if (Style.Language == FormatStyle::LK_Proto ||
bool TokenAnnotator::canBreakBefore(const AnnotatedLine &Line,
   if (Right.is(tok::r_square) && Right.MatchingParen &&
       Right.MatchingParen->is(TT_LambdaLSquare)) {
     return false;
   }
 
   // We only break before r_brace if there was a corresponding break before
   // the l_brace, which is tracked by BreakBeforeClosingBrace.
   if (Right.is(tok::r_brace))
-    return Right.MatchingParen && Right.MatchingParen->is(BK_Block);
+    return Right.MatchingParen && (Right.MatchingParen->is(BK_Block) ||
+      (Right.MatchingParen->isOneOf(BK_BracedInit, BK_ListInit) &&
+       Style.Cpp11BracedListStyle &&
+       Style.AlignAfterOpenBracket == FormatStyle::BAS_BlockIndent));
 
   // We only break before r_paren if we're in a block indented context.
   if (Right.is(tok::r_paren)) {
     if (Style.AlignAfterOpenBracket != FormatStyle::BAS_BlockIndent ||
         !Right.MatchingParen) {
       return false;
     }
     auto Next = Right.Next;

clang/lib/Format/UnwrappedLineParser.cpp

case tok::equal:
 
       nextToken();
       if (FormatTok->is(tok::l_brace)) {
         // Block kind should probably be set to BK_BracedInit for any language.
         // C# needs this change to ensure that array initialisers and object
         // initialisers are indented the same way.
         if (Style.isCSharp())
           FormatTok->setBlockKind(BK_BracedInit);
+        else if (Style.isCpp())
+          FormatTok->setBlockKind(BK_ListInit);
         nextToken();
         parseBracedList();
       } else if (Style.Language == FormatStyle::LK_Proto &&
                  FormatTok->is(tok::less)) {
         nextToken();
         parseBracedList(/*ContinueOnSemicolons=*/false, /*IsEnum=*/false,
                         /*ClosingBraceKind=*/tok::greater);
       }

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, BracedInitializerIndentWidth) {
   verifyFormat("int LooooooooooooooooooooooooongVariable[2] = {\n"
                "      10000000, 20000000};",
                Style);
   verifyFormat("SomeStruct s{\n"
                "      \"xxxxxxxxxxxxxxxx\", \"yyyyyyyyyyyyyyyy\",\n"
                "      \"zzzzzzzzzzzzzzzz\"};\n",
                Style);
   // Designated initializers.
-  verifyFormat("int LooooooooooooooooooooooooongVariable[1] = {\n"
+  verifyFormat("int LooooooooooooooooooooooooongVariable[2] = {\n"
                "      [0] = 10000000, [1] = 20000000};",
                Style);
   verifyFormat("SomeStruct s{\n"
                "      .foo = \"xxxxxxxxxxxxx\",\n"
                "      .bar = \"yyyyyyyyyyyyy\",\n"
                "      .baz = \"zzzzzzzzzzzzz\"};\n",
                Style);
   // List initialization.
verifyFormat("SomeArrayT a[3] = {\n"
                "            foo,\n"
                "            bar,\n"
                "      },\n"
                "      {\n"
                "            foo,\n"
                "            bar,\n"
                "      },\n"
                "      SomeArrayT{},\n"
-               "}\n",
+               "};\n",
                Style);
   verifyFormat("SomeArrayT a[3] = {\n"
                "      {foo},\n"
                "      {\n"
                "            {\n"
                "                  init1,\n"
                "                  init2,\n"
                "                  init3,\n"
                "            },\n"
                "            {\n"
                "                  init1,\n"
                "                  init2,\n"
                "                  init3,\n"
                "            },\n"
                "      },\n"
                "      {baz},\n"
-               "}\n",
+               "};\n",
                Style);
 
   // Aligning after open braces unaffected by BracedInitializerIndentWidth.
   Style.AlignAfterOpenBracket = FormatStyle::BAS_Align;
   verifyFormat("SomeStruct s{\"xxxxxxxxxxxxx\", \"yyyyyyyyyyyyy\",\n"
                "             \"zzzzzzzzzzzzz\"};\n",
                Style);
 }
TEST_F(FormatTest, AlignAfterOpenBracketBlockIndentForStatement) {
   verifyFormat("for (int myReallyLongCountVariable = 0; "
                "myReallyLongCountVariable < count;\n"
                "     myReallyLongCountVariable++) {\n"
                "  doSomething();\n"
                "}",
                Style);
 }
 
+TEST_F(FormatTest, AlignAfterOpenBracketBlockIndentInitializers) {
+  auto Style = getLLVMStyleWithColumns(60);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_BlockIndent;
+  // Aggregate initialization.
+  verifyFormat("int LooooooooooooooooooooooooongVariable[2] = {\n"
+               "    10000000, 20000000\n"
+               "};",
+               Style);
+  verifyFormat("SomeStruct s{\n"
+               "    \"xxxxxxxxxxxxxxxx\", \"yyyyyyyyyyyyyyyy\",\n"
+               "    \"zzzzzzzzzzzzzzzz\"\n"
+               "};",
+               Style);
+  // Designated initializers.
+  verifyFormat("int LooooooooooooooooooooooooongVariable[2] = {\n"
+               "    [0] = 10000000, [1] = 20000000\n"
+               "};",
+               Style);
+  verifyFormat("SomeStruct s{\n"
+               "    .foo = \"xxxxxxxxxxxxx\",\n"
+               "    .bar = \"yyyyyyyyyyyyy\",\n"
+               "    .baz = \"zzzzzzzzzzzzz\"\n"
+               "};\n",
+               Style);
+  // List initialization.
+  verifyFormat("SomeStruct s{\n"
+               "    \"xxxxxxxxxxxxx\",\n"
+               "    \"yyyyyyyyyyyyy\",\n"
+               "    \"zzzzzzzzzzzzz\",\n"
+               "};\n",
+               Style);
+  verifyFormat("SomeStruct{\n"
+               "    \"xxxxxxxxxxxxx\",\n"
+               "    \"yyyyyyyyyyyyy\",\n"
+               "    \"zzzzzzzzzzzzz\",\n"
+               "};\n",
+               Style);
+  verifyFormat("new SomeStruct{\n"
+               "    \"xxxxxxxxxxxxx\",\n"
+               "    \"yyyyyyyyyyyyy\",\n"
+               "    \"zzzzzzzzzzzzz\",\n"
+               "};\n",
+               Style);
+  // Member initializer.
+  verifyFormat("class SomeClass {\n"
+               "  SomeStruct s{\n"
+               "      \"xxxxxxxxxxxxx\",\n"
+               "      \"yyyyyyyyyyyyy\",\n"
+               "      \"zzzzzzzzzzzzz\",\n"
+               "  };\n"
+               "};\n",
+               Style);
+  // Constructor member initializer.
+  verifyFormat("SomeClass::SomeClass : strct{\n"
+               "                           \"xxxxxxxxxxxxx\",\n"
+               "                           \"yyyyyyyyyyyyy\",\n"
+               "                           \"zzzzzzzzzzzzz\",\n"
+               "                       } {}\n",
+               Style);
+  // Copy initialization.
+  verifyFormat("SomeStruct s = SomeStruct{\n"
+               "    \"xxxxxxxxxxxxx\",\n"
+               "    \"yyyyyyyyyyyyy\",\n"
+               "    \"zzzzzzzzzzzzz\",\n"
+               "};\n",
+               Style);
+  // Copy list initialization.
+  verifyFormat("SomeStruct s = {\n"
+               "    \"xxxxxxxxxxxxx\",\n"
+               "    \"yyyyyyyyyyyyy\",\n"
+               "    \"zzzzzzzzzzzzz\",\n"
+               "};\n",
+               Style);
+  // Assignment operand initialization.
+  verifyFormat("s = {\n"
+               "    \"xxxxxxxxxxxxx\",\n"
+               "    \"yyyyyyyyyyyyy\",\n"
+               "    \"zzzzzzzzzzzzz\",\n"
+               "};\n",
+               Style);
+  // Returned object initialization.
+  verifyFormat("return {\n"
+               "    \"xxxxxxxxxxxxx\",\n"
+               "    \"yyyyyyyyyyyyy\",\n"
+               "    \"zzzzzzzzzzzzz\",\n"
+               "};\n",
+               Style);
+  // Initializer list.
+  verifyFormat("auto initializerList = {\n"
+               "    \"xxxxxxxxxxxxx\",\n"
+               "    \"yyyyyyyyyyyyy\",\n"
+               "    \"zzzzzzzzzzzzz\",\n"
+               "};\n",
+               Style);
+  // Function parameter initialization.
+  verifyFormat("func({\n"
+               "    \"xxxxxxxxxxxxx\",\n"
+               "    \"yyyyyyyyyyyyy\",\n"
+               "    \"zzzzzzzzzzzzz\",\n"
+               "});\n",
+               Style);
+  // Nested init lists.
+  verifyFormat("SomeStruct s = {\n"
+               "    {{init1, init2, init3, init4, init5},\n"
+               "     {init1, init2, init3, init4, init5}}\n"
+               "};\n",
+               Style);
+  verifyFormat("SomeStruct s = {\n"
+               "    {{\n"
+               "         .init1 = 1,\n"
+               "         .init2 = 2,\n"
+               "         .init3 = 3,\n"
+               "         .init4 = 4,\n"
+               "         .init5 = 5,\n"
+               "     },\n"
+               "     {init1, init2, init3, init4, init5}}\n"
+               "};\n",
+               Style);
+  verifyFormat("SomeArrayT a[3] = {\n"
+               "    {\n"
+               "        foo,\n"
+               "        bar,\n"
+               "    },\n"
+               "    {\n"
+               "        foo,\n"
+               "        bar,\n"
+               "    },\n"
+               "    SomeArrayT{},\n"
+               "};\n",
+               Style);
+  verifyFormat("SomeArrayT a[3] = {\n"
+               "    {foo},\n"
+               "    {\n"
+               "        {\n"
+               "            init1,\n"
+               "            init2,\n"
+               "            init3,\n"
+               "        },\n"
+               "        {\n"
+               "            init1,\n"
+               "            init2,\n"
+               "            init3,\n"
+               "        },\n"
+               "    },\n"
+               "    {baz},\n"
+               "};\n",
+               Style);
+}
+
 TEST_F(FormatTest, UnderstandsDigraphs) {
   verifyFormat("int arr<:5:> = {};");
   verifyFormat("int arr[5] = <%%>;");
   verifyFormat("int arr<:::qualified_variable:> = {};");
   verifyFormat("int arr[::qualified_variable] = <%%>;");
   verifyFormat("%:include <header>");
   verifyFormat("%:define A x##y");
   verifyFormat("#define A x%:%:y");