[clang-format] Fix AlignArrayOfStructures + Cpp11BracedListStyle=false

[galenelias]

Summary

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

Currently AlignArrayOfStructures=Left is hard coding setting Spaces to
0 for the token following the initial opening brace, but not touching
Spaces for the subsequent lines, which leads to the array being
misaligned. Additionally, it's not adding a space before the trailing
} which is generally done when Cpp11BracedListStyle=false.

I'm not exactly sure why this function needs to override the Spaces as
it seems to generally already be set to either 0 or 1 according to
the other formatting settings, but I'm going with an explicit fix where
I just force the padding to 1 when Cpp11BracedListStyle=false.

AlignArrayOfStructures=Right doesn't have any alignment problems, but
isn't adding the expected padding around the braces either, so I'm
giving that the same treatment.

[owenpan]

I'm not exactly sure why this function needs to override the Spaces as it seems to generally already be set to either 0 or 1 according to the other formatting settings

If so, can we address the issue without the "explicit fix"?

clang/lib/Format/WhitespaceManager.cpp

1233

1251

Can we assert that Spaces == 0? If not, we should add a test case.

1307

Ditto.

clang/unittests/Format/FormatTest.cpp

20659

20899

Ditto.

[owenpan]

clang/lib/Format/WhitespaceManager.cpp

1251

We can't assert that, but setting Spaces here seems superfluous as it's set correctly below anyways?

[galenelias]

clang/lib/Format/WhitespaceManager.cpp

1251

I admit I'm not super confident on my understanding of this code, but this setting of Spaces is not redundant with any below settings. If we set it to '3' for instance, that won't get overwritten later (because the other sets are all conditional, and don't hit for the } token).

So, I think this is still required (minus the issue of the existing 'Spaces' calculation from previous formatting pass seemingly already setting Spaces to the correct value).

clang/unittests/Format/FormatTest.cpp

20659

This is consistent with basically every single adjacent test in this function. While I agree that this is unnecessary, in general I error on the side of consistency with the surrounding tests. I'll defer to the maintainers, just wanted to make sure this is actually the preferred change given the numerous adjacent tests with this form.

[owenpan]

clang/unittests/Format/FormatTest.cpp

20659

If you rebase your patch, you'll see that the trailing newlines in the surrounding tests have been removed. (Even if they had not been removed, we still wouldn't want new tests to have superfluous trailing newlines.)

[galenelias]

clang/unittests/Format/FormatTest.cpp

20659

Thanks for the information, looks like I missed your cleanup change by a day. In general some code-bases favor consistency over some extra gunk, so I tend to default to consistency, but good to know going forward. I've rebased now.

[owenpan]

clang/unittests/Format/FormatTest.cpp

20659

Np!

clang/lib/Format/WhitespaceManager.cpp

else if (Style.AlignArrayOfStructures == FormatStyle::AIAS_Left)
     alignArrayInitializersLeftJustified(getCells(Start, End));
 }
 
 void WhitespaceManager::alignArrayInitializersRightJustified(
     CellDescriptions &&CellDescs) {
   if (!CellDescs.isRectangular())
     return;
 
+  const int BracePadding = Style.Cpp11BracedListStyle ? 0 : 1;
   auto &Cells = CellDescs.Cells;
   // Now go through and fixup the spaces.
   auto *CellIter = Cells.begin();
   for (auto i = 0U; i < CellDescs.CellCounts[0]; ++i, ++CellIter) {
     unsigned NetWidth = 0U;
     if (isSplitCell(*CellIter))
       NetWidth = getNetWidth(Cells.begin(), CellIter, CellDescs.InitialSpaces);
     auto CellWidth = getMaximumCellWidth(CellIter, NetWidth);
 
     if (Changes[CellIter->Index].Tok->is(tok::r_brace)) {
       // So in here we want to see if there is a brace that falls
       // on a line that was split. If so on that line we make sure that
       // the spaces in front of the brace are enough.
       const auto *Next = CellIter;
       do {
         const FormatToken *Previous = Changes[Next->Index].Tok->Previous;
         if (Previous && Previous->isNot(TT_LineComment)) {
-          Changes[Next->Index].Spaces = 0;
+          Changes[Next->Index].Spaces = BracePadding;
           Changes[Next->Index].NewlinesBefore = 0;
         }
         Next = Next->NextColumnElement;
       } while (Next);
       // Unless the array is empty, we need the position of all the
       // immediately adjacent cells
       if (CellIter != Cells.begin()) {
         auto ThisNetWidth =
if (Changes[CellIter->Index].Tok->is(tok::r_brace)) {
         }
       }
     } else {
       auto ThisWidth =
           calculateCellWidth(CellIter->Index, CellIter->EndIndex, true) +
           NetWidth;
       if (Changes[CellIter->Index].NewlinesBefore == 0) {
         Changes[CellIter->Index].Spaces = (CellWidth - (ThisWidth + NetWidth));
-        Changes[CellIter->Index].Spaces += (i > 0) ? 1 : 0;
+        Changes[CellIter->Index].Spaces += (i > 0) ? 1 : BracePadding;
       }
       alignToStartOfCell(CellIter->Index, CellIter->EndIndex);
       for (const auto *Next = CellIter->NextColumnElement; Next;
            Next = Next->NextColumnElement) {
         ThisWidth =
             calculateCellWidth(Next->Index, Next->EndIndex, true) + NetWidth;
         if (Changes[Next->Index].NewlinesBefore == 0) {
           Changes[Next->Index].Spaces = (CellWidth - ThisWidth);
-          Changes[Next->Index].Spaces += (i > 0) ? 1 : 0;
+          Changes[Next->Index].Spaces += (i > 0) ? 1 : BracePadding;
         }
         alignToStartOfCell(Next->Index, Next->EndIndex);
       }
     }
   }
 }
 
 void WhitespaceManager::alignArrayInitializersLeftJustified(
     CellDescriptions &&CellDescs) {
 
   if (!CellDescs.isRectangular())
     return;
 
+  const int BracePadding = Style.Cpp11BracedListStyle ? 0 : 1;
   auto &Cells = CellDescs.Cells;
   // Now go through and fixup the spaces.
   auto *CellIter = Cells.begin();
   // The first cell needs to be against the left brace.
   if (Changes[CellIter->Index].NewlinesBefore == 0)
-    Changes[CellIter->Index].Spaces = 0;
+    Changes[CellIter->Index].Spaces = BracePadding;
   else
     Changes[CellIter->Index].Spaces = CellDescs.InitialSpaces;
   ++CellIter;
   for (auto i = 1U; i < CellDescs.CellCounts[0]; i++, ++CellIter) {
     auto MaxNetWidth = getMaximumNetWidth(
         Cells.begin(), CellIter, CellDescs.InitialSpaces,
         CellDescs.CellCounts[0], CellDescs.CellCounts.size());
     auto ThisNetWidth =
         getNetWidth(Cells.begin(), CellIter, CellDescs.InitialSpaces);
     if (Changes[CellIter->Index].NewlinesBefore == 0) {
       Changes[CellIter->Index].Spaces =
           MaxNetWidth - ThisNetWidth +
-          (Changes[CellIter->Index].Tok->isNot(tok::r_brace) ? 1 : 0);
+          (Changes[CellIter->Index].Tok->isNot(tok::r_brace) ? 1
+                                                             : BracePadding);
     }
     auto RowCount = 1U;
     auto Offset = std::distance(Cells.begin(), CellIter);
     for (const auto *Next = CellIter->NextColumnElement; Next;
          Next = Next->NextColumnElement) {
       if (RowCount > CellDescs.CellCounts.size())
         break;
       auto *Start = (Cells.begin() + RowCount * CellDescs.CellCounts[0]);
       auto *End = Start + Offset;
       auto ThisNetWidth = getNetWidth(Start, End, CellDescs.InitialSpaces);
       if (Changes[Next->Index].NewlinesBefore == 0) {
         Changes[Next->Index].Spaces =
             MaxNetWidth - ThisNetWidth +
-            (Changes[Next->Index].Tok->isNot(tok::r_brace) ? 1 : 0);
+            (Changes[Next->Index].Tok->isNot(tok::r_brace) ? 1 : BracePadding);
       }
       ++RowCount;
     }
   }
 }
 
 bool WhitespaceManager::isSplitCell(const CellDescription &Cell) {
   if (Cell.HasSplit)

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, CatchAlignArrayOfStructuresRightAlignment) {
   verifyFormat("return GradForUnaryCwise(g, {\n"
                "                                {{\"sign\"}, \"Sign\",  "
                "  {\"x\", \"dy\"}},\n"
                "                                {  {\"dx\"},  \"Mul\", {\"dy\""
                ", \"sign\"}},\n"
                "});",
                Style);
 
+  Style.Cpp11BracedListStyle = false;
+  verifyFormat("struct test demo[] = {\n"
+               "  { 56,    23, \"hello\" },\n"
+               "  { -1, 93463, \"world\" },\n"
+               "  {  7,     5,    \"!!\" }\n"
+               "};",
+               Style);
+  Style.Cpp11BracedListStyle = true;
+
   Style.ColumnLimit = 0;
   verifyFormat(
       "test demo[] = {\n"
       "    {56,    23, \"hello world i am a very long line that really, "
       "in any just world, ought to be split over multiple lines\"},\n"
       "    {-1, 93463,                                                  "
       "                                                 \"world\"},\n"
       "    { 7,     5,                                                  "
TEST_F(FormatTest, CatchAlignArrayOfStructuresLeftAlignment) {
   verifyFormat("return GradForUnaryCwise(g, {\n"
                "                                {{\"sign\"}, \"Sign\", {\"x\", "
                "\"dy\"}   },\n"
                "                                {{\"dx\"},   \"Mul\",  "
                "{\"dy\", \"sign\"}},\n"
                "});",
                Style);
 
+  Style.Cpp11BracedListStyle = false;
+  verifyFormat("struct test demo[] = {\n"
+               "  { 56, 23,    \"hello\" },\n"
+               "  { -1, 93463, \"world\" },\n"
+               "  { 7,  5,     \"!!\"    }\n"
+               "};",
+               Style);
+  Style.Cpp11BracedListStyle = true;
+
   Style.ColumnLimit = 0;
   verifyFormat(
       "test demo[] = {\n"
       "    {56, 23,    \"hello world i am a very long line that really, in any "
       "just world, ought to be split over multiple lines\"},\n"
       "    {-1, 93463, \"world\"                                               "
       "                                                   },\n"
       "    {7,  5,     \"!!\"                                                  "