[clang-format] fix alignment w/o binpacked args

[cha5on]

Summary

The combination of

• AlignConsecutiveAssignments.Enabled = true
• BinPackArguments = false

would result in the first continuation line of a braced-init-list being
improperly indented (missing a shift) when in a continued function call.
Indentation was also wrong for braced-init-lists continuing a
direct-list-initialization. Check for opening braced lists in
continuation and ensure that the correct shift occurs.

Fixes https://github.com/llvm/llvm-project/issues/55360

[HazardyKnusperkeks]

For me there needs no bug report, but could you comment on the misformatting without the patch?

clang/unittests/Format/FormatTest.cpp

17327

Drop the scope, we don't have that in other tests.

[curdeius]

Thanks for working on this!
Is there a bug report somewhere? If not, may you create one please?

clang/lib/Format/WhitespaceManager.cpp

398

Elide braces please.

clang/unittests/Format/FormatTest.cpp

17359

I don't think you need to create variables, just online the strings.

Also, could you minimize (and possibly split) as much as possible these test cases?

[curdeius]

FYI, I like to have a bug report because people that encounter the same problem (in an older release) can find it easily, and see if/when it was fixed. It also avoid working on the same thing by different people.

[cha5on]

Update for review

• Add and link bug report.
• Remove braces per LLVM style.
• Shrink down test case and add second case for direct-list-initialization alignment.

[cha5on]

clang/unittests/Format/FormatTest.cpp

17359

Shrank this down and added a second testcase for direct-list-initialization.

[curdeius]

Thanks for creating the bug report. A few more comments.

clang/lib/Format/WhitespaceManager.cpp

374–375

It seems that we set BK_BracedInit only on l_brace, so no need for a redundant check.

394–398

Ditto.

clang/unittests/Format/FormatTest.cpp

17328–17337

Why not just this?

17338–17347

And here.

[HazardyKnusperkeks]

clang/unittests/Format/FormatTest.cpp

17324

I'd drop that, or at least replace it with https://llvm.org/PR55360.

[cha5on]

Update for review.

• Further simplify testcases.
• Update issue URL in comment to use llvm.org alias.

clang/lib/Format/WhitespaceManager.cpp

374–375

BK_BracedInit also gets set on r_brace, assuming I'm reading clang/lib/Format/UnwrappedLineParser.cpp correctly. I think we need both to be true to be sure that this is the intended token.

clang/unittests/Format/FormatTest.cpp

17324

Replaced.

17328–17337

I was reading a comment in this file that says that messUp manipulates leading whitespace and thought that meant it wouldn't be appropriate for this. Upon a closer look, it seems to work fine here, so updating to just provide the expected case.

[curdeius]

LGTM. Tell us if you need help landing this.
Thanks a lot for your contribution!

clang/lib/Format/WhitespaceManager.cpp

374–375

Ok.

[cha5on]

In D125162#3510737, @curdeius wrote:

LGTM. Tell us if you need help landing this.

I don't have commit access, so some help would be great :-)

clang/lib/Format/WhitespaceManager.cpp

if (InsideNestedScope && Changes[i].NewlinesBefore > 0) {
             Changes[ScopeStart - 2].Tok->is(tok::identifier) &&
             Changes[ScopeStart - 1].Tok->is(tok::l_paren) &&
             Changes[ScopeStart].Tok->isNot(TT_LambdaLSquare)) {
           if (Changes[i].Tok->MatchingParen &&
               Changes[i].Tok->MatchingParen->is(TT_LambdaLBrace))
             return false;
           if (Changes[ScopeStart].NewlinesBefore > 0)
             return false;
+          if (Changes[i].Tok->is(tok::l_brace) &&
+              Changes[i].Tok->is(BK_BracedInit))
+            return true;
           return Style.BinPackArguments;
         }
 
         // Ternary operator
         if (Changes[i].Tok->is(TT_ConditionalExpr))
           return true;
 
         // Period Initializer .XXX = 1.
         if (Changes[i].Tok->is(TT_DesignatedInitializerPeriod))
           return true;
 
         // Continued ternary operator
         if (Changes[i].Tok->Previous &&
             Changes[i].Tok->Previous->is(TT_ConditionalExpr))
           return true;
 
+        // Continued direct-list-initialization using braced list.
+        if (ScopeStart > Start + 1 &&
+            Changes[ScopeStart - 2].Tok->is(tok::identifier) &&
+            Changes[ScopeStart - 1].Tok->is(tok::l_brace) &&
+            Changes[i].Tok->is(tok::l_brace) &&
+            Changes[i].Tok->is(BK_BracedInit))
+          return true;
+
         // Continued braced list.
         if (ScopeStart > Start + 1 &&
             Changes[ScopeStart - 2].Tok->isNot(tok::identifier) &&
             Changes[ScopeStart - 1].Tok->is(tok::l_brace) &&
             Changes[i].Tok->isNot(tok::r_brace)) {
           for (unsigned OuterScopeStart : llvm::reverse(ScopeStack)) {
             // Lambda.
             if (OuterScopeStart > Start &&

clang/unittests/Format/FormatTest.cpp

verifyFormat("auto b = f(aaaaaaaaaaaaaaaaaaaaaaaaa,\n"
                "           dddddddddddddddddddddddddd);",
                Alignment);
   // FIXME: https://llvm.org/PR53497
   // verifyFormat("auto aaaaaaaaaaaa = f();\n"
   //              "auto b            = f(aaaaaaaaaaaaaaaaaaaaaaaaa,\n"
   //              "    ccc ? aaaaa : bbbbb,\n"
   //              "    dddddddddddddddddddddddddd);",
   //              Alignment);
+
+  // Confirm proper handling of AlignConsecutiveAssignments with
+  // BinPackArguments.
+  // See https://llvm.org/PR55360
+  Alignment = getLLVMStyleWithColumns(50);
+  Alignment.AlignConsecutiveAssignments.Enabled = true;
+  Alignment.BinPackArguments = false;
+  verifyFormat("int a_long_name = 1;\n"
+               "auto b          = B({a_long_name, a_long_name},\n"
+               "                    {a_longer_name_for_wrap,\n"
+               "                     a_longer_name_for_wrap});",
+               Alignment);
+  verifyFormat("int a_long_name = 1;\n"
+               "auto b          = B{{a_long_name, a_long_name},\n"
+               "                    {a_longer_name_for_wrap,\n"
+               "                     a_longer_name_for_wrap}};",
+               Alignment);
 }
 
 TEST_F(FormatTest, AlignConsecutiveBitFields) {
   FormatStyle Alignment = getLLVMStyle();
   Alignment.AlignConsecutiveBitFields.Enabled = true;
   verifyFormat("int const a     : 5;\n"
                "int oneTwoThree : 23;",
                Alignment);
 
   // Initializers are allowed starting with c++2a
   verifyFormat("int const a     : 5 = 1;\n"
                "int oneTwoThree : 23 = 0;",
                Alignment);
 
   Alignment.AlignConsecutiveDeclarations.Enabled = true;
   verifyFormat("int const a           : 5;\n"
                "int       oneTwoThree : 23;",
                Alignment);
 
   verifyFormat("int const a           : 5;  // comment\n"
                "int       oneTwoThree : 23; // comment",
                Alignment);
 
   verifyFormat("int const a           : 5 = 1;\n"
                "int       oneTwoThree : 23 = 0;",
                Alignment);
 
   Alignment.AlignConsecutiveAssignments.Enabled = true;
   verifyFormat("int const a           : 5  = 1;\n"
                "int       oneTwoThree : 23 = 0;",