Skip to content

Commit ec4829e

Browse files
feg208tstellar
authored andcommitted
fixes bug llvm#51926 where dangling comma caused overrun
bug 51926 identified an issue where a dangling comma caused the cell count to be to off by one Differential Revision: https://reviews.llvm.org/D110481 (cherry picked from commit a36227c)
1 parent 73daeb3 commit ec4829e

File tree

2 files changed

+27
-3
lines changed

2 files changed

+27
-3
lines changed

clang/lib/Format/WhitespaceManager.cpp

+14-2
Original file line numberDiff line numberDiff line change
@@ -1146,14 +1146,15 @@ WhitespaceManager::CellDescriptions WhitespaceManager::getCells(unsigned Start,
11461146
} else if (C.Tok->is(tok::comma)) {
11471147
if (!Cells.empty())
11481148
Cells.back().EndIndex = i;
1149-
Cell++;
1149+
if (C.Tok->getNextNonComment()->isNot(tok::r_brace)) // dangling comma
1150+
++Cell;
11501151
}
11511152
} else if (Depth == 1) {
11521153
if (C.Tok == MatchingParen) {
11531154
if (!Cells.empty())
11541155
Cells.back().EndIndex = i;
11551156
Cells.push_back(CellDescription{i, ++Cell, i + 1, false, nullptr});
1156-
CellCount = Cell + 1;
1157+
CellCount = C.Tok->Previous->isNot(tok::comma) ? Cell + 1 : Cell;
11571158
// Go to the next non-comment and ensure there is a break in front
11581159
const auto *NextNonComment = C.Tok->getNextNonComment();
11591160
while (NextNonComment->is(tok::comma))
@@ -1190,6 +1191,17 @@ WhitespaceManager::CellDescriptions WhitespaceManager::getCells(unsigned Start,
11901191
// So if we split a line previously and the tail line + this token is
11911192
// less then the column limit we remove the split here and just put
11921193
// the column start at a space past the comma
1194+
//
1195+
// FIXME This if branch covers the cases where the column is not
1196+
// the first column. This leads to weird pathologies like the formatting
1197+
// auto foo = Items{
1198+
// Section{
1199+
// 0, bar(),
1200+
// }
1201+
// };
1202+
// Well if it doesn't lead to that it's indicative that the line
1203+
// breaking should be revisited. Unfortunately alot of other options
1204+
// interact with this
11931205
auto j = i - 1;
11941206
if ((j - 1) > Start && Changes[j].Tok->is(tok::comma) &&
11951207
Changes[j - 1].NewlinesBefore > 0) {

clang/unittests/Format/FormatTest.cpp

+13-1
Original file line numberDiff line numberDiff line change
@@ -17784,13 +17784,25 @@ TEST_F(FormatTest, CatchAlignArrayOfStructuresRightAlignment) {
1778417784
TEST_F(FormatTest, CatchAlignArrayOfStructuresLeftAlignment) {
1778517785
auto Style = getLLVMStyle();
1778617786
Style.AlignArrayOfStructures = FormatStyle::AIAS_Left;
17787+
/* FIXME: This case gets misformatted.
17788+
verifyFormat("auto foo = Items{\n"
17789+
" Section{0, bar(), },\n"
17790+
" Section{1, boo() }\n"
17791+
"};\n",
17792+
Style);
17793+
*/
17794+
verifyFormat("auto foo = Items{\n"
17795+
" Section{\n"
17796+
" 0, bar(),\n"
17797+
" }\n"
17798+
"};\n",
17799+
Style);
1778717800
verifyFormat("struct test demo[] = {\n"
1778817801
" {56, 23, \"hello\"},\n"
1778917802
" {-1, 93463, \"world\"},\n"
1779017803
" {7, 5, \"!!\" }\n"
1779117804
"};\n",
1779217805
Style);
17793-
1779417806
verifyFormat("struct test demo[] = {\n"
1779517807
" {56, 23, \"hello\"}, // first line\n"
1779617808
" {-1, 93463, \"world\"}, // second line\n"

0 commit comments

Comments
 (0)