[clang-format] Fix crashes in AlignArrayOfStructures #72520
Conversation
|
@llvm/pr-subscribers-clang-format Author: Owen Pan (owenca) ChangesFixed #55493. Full diff: https://github.com/llvm/llvm-project/pull/72520.diff 3 Files Affected:
diff --git a/clang/lib/Format/WhitespaceManager.cpp b/clang/lib/Format/WhitespaceManager.cpp
index 32d8b97cc8dadb1..3bc6915b8df0a70 100644
--- a/clang/lib/Format/WhitespaceManager.cpp
+++ b/clang/lib/Format/WhitespaceManager.cpp
@@ -1316,6 +1316,8 @@ void WhitespaceManager::alignArrayInitializersRightJustified(
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;
ThisNetWidth = getNetWidth(Start, End, CellDescs.InitialSpaces);
@@ -1379,7 +1381,7 @@ void WhitespaceManager::alignArrayInitializersLeftJustified(
auto Offset = std::distance(Cells.begin(), CellIter);
for (const auto *Next = CellIter->NextColumnElement; Next;
Next = Next->NextColumnElement) {
- if (RowCount > CellDescs.CellCounts.size())
+ if (RowCount >= CellDescs.CellCounts.size())
break;
auto *Start = (Cells.begin() + RowCount * CellDescs.CellCounts[0]);
auto *End = Start + Offset;
diff --git a/clang/lib/Format/WhitespaceManager.h b/clang/lib/Format/WhitespaceManager.h
index df7e9add1cd446f..69398fe411502f1 100644
--- a/clang/lib/Format/WhitespaceManager.h
+++ b/clang/lib/Format/WhitespaceManager.h
@@ -317,7 +317,7 @@ class WhitespaceManager {
auto Offset = std::distance(CellStart, CellStop);
for (const auto *Next = CellStop->NextColumnElement; Next;
Next = Next->NextColumnElement) {
- if (RowCount > MaxRowCount)
+ if (RowCount >= MaxRowCount)
break;
auto Start = (CellStart + RowCount * CellCount);
auto End = Start + Offset;
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index a12a20359c2fad2..cd4c93e8427723f 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -21140,6 +21140,24 @@ TEST_F(FormatTest, CatchAlignArrayOfStructuresLeftAlignment) {
"that really, in any just world, ought to be split over multiple "
"lines\"},{-1, 93463, \"world\"},{7, 5, \"!!\"},};",
Style);
+
+ Style.ColumnLimit = 25;
+ verifyNoCrash("Type object[X][Y] = {\n"
+ " {{val}, {val}, {val}},\n"
+ " {{val}, {val}, // some comment\n"
+ " {val}}\n"
+ "};",
+ Style);
+
+ Style.ColumnLimit = 120;
+ verifyNoCrash(
+ "T v[] {\n"
+ " { AAAAAAAAAAAAAAAAAAAAAAAAA::aaaaaaaaaaaaaaaaaaa, "
+ "AAAAAAAAAAAAAAAAAAAAAAAAA::aaaaaaaaaaaaaaaaaaaaaaaa, 1, 0.000000000f, "
+ "\"00000000000000000000000000000000000000000000000000000000"
+ "00000000000000000000000000000000000000000000000000000000\" },\n"
+ "};",
+ Style);
}
TEST_F(FormatTest, UnderstandsPragmas) {
|
| for (const auto *Next = CellIter->NextColumnElement; Next; | ||
| Next = Next->NextColumnElement) { | ||
| if (RowCount >= CellDescs.CellCounts.size()) | ||
| break; |
There was a problem hiding this comment.
| for (const auto *Next = CellIter->NextColumnElement; Next; | |
| Next = Next->NextColumnElement) { | |
| if (RowCount >= CellDescs.CellCounts.size()) | |
| break; | |
| for (const auto *Next = CellIter->NextColumnElement; Next && RowCount < CellDescs.CellCounts.size(); | |
| Next = Next->NextColumnElement, ++RowCount) { |
Maybe?
Then the ++RowCount below must be removed.
There was a problem hiding this comment.
I intentionally made it the same as lines 1384-1385 below so that lines 1315-1323 would remain copy-pasted from lines 1380-1388. This will make future refactoring easier.
There was a problem hiding this comment.
I would change the other lines too. I think it's better to see the condition in the for, but it's your call.
There was a problem hiding this comment.
The same/similar code appeared twice in this file and once in the header file. If I were to refactor it, I might not use the same for-loop, so I'll leave it as is for now. (There are also much more cleanup that can be done with this option, but I don't know if it's worthwhile as it's so buggy even after @mydeveloperday limited it to matrices.)
|
We probably should backport it to 17.0.6. What do you all think? @tru |
Yep - seems like a good and small fix to have in 17.x |
Any idea if there is another workaround or fix that we could do to target 17.x? 18 is still pretty far off and clang-format for 17 will soon be included in a lot of downstream tools. Happy to help out fixing it if you have any pointers. |
My bad. #72628 was already "fixed" on 18 before this patch. Can you help bisect it? |
Fixed llvm#54815. Fixed llvm#55269. Fixed llvm#55493. Fixed llvm#68431.
Fixed llvm#54815. Fixed llvm#55269. Fixed llvm#55493. Fixed llvm#68431.
Fixed #54815.
Fixed #55269.
Fixed #55493.
Fixed #68431.