fixes bug #51926 where dangling comma caused overrun

[feg208]

Summary

bug 51926 identified an issue where a dangling comma caused the cell count to be to off by one

[HazardyKnusperkeks]

clang/unittests/Format/FormatTest.cpp

17805

Here is no alignment, maybe add another case with another line?

What happens if one line has the trailing comma and the next doesn't?

[feg208]

clang/unittests/Format/FormatTest.cpp

17805

yesh this is pretty ugly. In the second case it crashes. This isn't going to be fixed for a bit

[HazardyKnusperkeks]

clang/unittests/Format/FormatTest.cpp

17805

What is that bit? Now we can still revert the original change for LLVM 13 (at least I think so, since there are new RC Tags on GitHub), or can fix the crash.

[feg208]

clang/unittests/Format/FormatTest.cpp

17805

I think we should fix the crash since it’s only an issue with the dangling comma but if the consensus is we should revert then I can do that as well

[HazardyKnusperkeks]

clang/unittests/Format/FormatTest.cpp

17805

Totally agree, could you please add the test cases? The ones that crash you could comment out and fix afterwards.

I don't know how much time we have before the next version is released.

[feg208]

clang/unittests/Format/FormatTest.cpp

17805

Let me see if I can fix the weirdness with the second test case but I'll time box it to two days so this doesn't drag out. I'd feel better about it not crashing just because we added an additional row.

[feg208]

Gets rid of the crash for the second added case

[feg208]

So I added in the second case but commented it out as it gets misformatted. However it doesn't assert which was the alarming outcome. I have one other bug to clean up here and I am going to open a bug to fix the line breaking after initial { in certain cases since the code there needs to be revisited. I am doing formatting in the info gathering loop where really I should add all that in the back end. But thats a bigger change that I'd like to do outside of mitigating this users crash.

[feg208]

By one other bug I mean an entirely separate bug that I will handle in a separate PR. Sorry if that was confusing.

[HazardyKnusperkeks]

In D110481#3027289, @feg208 wrote:

By one other bug I mean an entirely separate bug that I will handle in a separate PR. Sorry if that was confusing.

Okay, but this should be handled quickly.

[feg208]

Absolutely in agreement

clang/lib/Format/WhitespaceManager.cpp

if (Depth == 2) {
           if (C.NewlinesBefore == 0) {
             InitialSpaces += Changes[j].Spaces + Changes[j].TokenLength;
             InitialTokenLength += Changes[j].TokenLength;
           }
         }
       } else if (C.Tok->is(tok::comma)) {
         if (!Cells.empty())
           Cells.back().EndIndex = i;
-        Cell++;
+        if (C.Tok->getNextNonComment()->isNot(tok::r_brace)) // dangling comma
+          ++Cell;
       }
     } else if (Depth == 1) {
       if (C.Tok == MatchingParen) {
         if (!Cells.empty())
           Cells.back().EndIndex = i;
         Cells.push_back(CellDescription{i, ++Cell, i + 1, false, nullptr});
-        CellCount = Cell + 1;
+        CellCount = C.Tok->Previous->isNot(tok::comma) ? Cell + 1 : Cell;
         // Go to the next non-comment and ensure there is a break in front
         const auto *NextNonComment = C.Tok->getNextNonComment();
         while (NextNonComment->is(tok::comma))
           NextNonComment = NextNonComment->getNextNonComment();
         auto j = i;
         while (Changes[j].Tok != NextNonComment && j < End)
           j++;
         if (j < End && Changes[j].NewlinesBefore == 0 &&
for (unsigned i = Start; i < End; ++i) {
     if (C.Tok->StartsColumn) {
       // This gets us past tokens that have been split over multiple
       // lines
       bool HasSplit = false;
       if (Changes[i].NewlinesBefore > 0) {
         // So if we split a line previously and the tail line + this token is
         // less then the column limit we remove the split here and just put
         // the column start at a space past the comma
+        //
+        // FIXME This if branch covers the cases where the column is not
+        // the first column. This leads to weird pathologies like the formatting
+        // auto foo = Items{
+        //     Section{
+        //             0, bar(),
+        //     }
+        // };
+        // Well if it doesn't lead to that it's indicative that the line
+        // breaking should be revisited. Unfortunately alot of other options
+        // interact with this
         auto j = i - 1;
         if ((j - 1) > Start && Changes[j].Tok->is(tok::comma) &&
             Changes[j - 1].NewlinesBefore > 0) {
           --j;
           auto LineLimit = Changes[j].Spaces + Changes[j].TokenLength;
           if (LineLimit < Style.ColumnLimit) {
             Changes[i].NewlinesBefore = 0;
             Changes[i].Spaces = 1;

clang/unittests/Format/FormatTest.cpp

EXPECT_EQ(
              "that really, in any just world, ought to be split over multiple "
              "lines\", 23},{-1, \"world\", 93463},{7, \"!!\", 5},};",
              Style));
 }
 
 TEST_F(FormatTest, CatchAlignArrayOfStructuresLeftAlignment) {
   auto Style = getLLVMStyle();
   Style.AlignArrayOfStructures = FormatStyle::AIAS_Left;
+  /* FIXME: This case gets misformatted.
+  verifyFormat("auto foo = Items{\n"
+               "    Section{0, bar(), },\n"
+               "    Section{1, boo()  }\n"
+               "};\n",
+               Style);
+  */
+  verifyFormat("auto foo = Items{\n"
+               "    Section{\n"
+               "            0, bar(),\n"
+               "            }\n"
+               "};\n",
+               Style);
   verifyFormat("struct test demo[] = {\n"
                "    {56, 23,    \"hello\"},\n"
                "    {-1, 93463, \"world\"},\n"
                "    {7,  5,     \"!!\"   }\n"
                "};\n",
                Style);
-
   verifyFormat("struct test demo[] = {\n"
                "    {56, 23,    \"hello\"}, // first line\n"
                "    {-1, 93463, \"world\"}, // second line\n"
                "    {7,  5,     \"!!\"   }  // third line\n"
                "};\n",
                Style);
   verifyFormat("struct test demo[4] = {\n"
                "    {56,  23,    21, \"oh\"      }, // first line\n"