MyDeveloperDay updated this revision to Diff 413301.
MyDeveloperDay marked 2 inline comments as done.
MyDeveloperDay added a comment.
There were no checks for if the cell would run off the end of the CellDescs
structure, this is part of the reason for the crashes that the PrevIter->Index
becomes invalid and is then used to index into the Changes.
The code just assumed it knew how many rows and columns there were, but in some
circumstances it gets it wrong and just keeps going. This MaxRows check should
at least prevent that by ensuring we just don't keep going off the end of the
rows. (this was the final crash)
Added a warning to the documentation.
F22348813: image.png <https://reviews.llvm.org/F22348813>
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121069/new/
https://reviews.llvm.org/D121069
Files:
clang/docs/ClangFormatStyleOptions.rst
clang/include/clang/Format/Format.h
clang/lib/Format/WhitespaceManager.cpp
clang/lib/Format/WhitespaceManager.h
clang/unittests/Format/FormatTest.cpp
Index: clang/unittests/Format/FormatTest.cpp
===================================================================
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -25298,6 +25298,138 @@
verifyFormat("#define A x%:%:y");
}
+TEST_F(FormatTest, AlignArrayOfStructuresLeftAlignmentNonSquare) {
+ auto Style = getLLVMStyle();
+ Style.AlignArrayOfStructures = FormatStyle::AIAS_Left;
+ Style.AlignConsecutiveAssignments =
+ FormatStyle::AlignConsecutiveStyle::ACS_Consecutive;
+ Style.AlignConsecutiveDeclarations =
+ FormatStyle::AlignConsecutiveStyle::ACS_Consecutive;
+
+ // The AlignArray code is incorrect for non square Arrays and can cause
+ // crashes, these tests assert that the array is not changed but will
+ // also act as regression tests for when it is properly fixed
+ verifyFormat("struct test demo[] = {\n"
+ " {1, 2},\n"
+ " {3, 4, 5},\n"
+ " {6, 7, 8}\n"
+ "};\n",
+ Style);
+ verifyFormat("struct test demo[] = {\n"
+ " {1, 2, 3, 4, 5},\n"
+ " {3, 4, 5},\n"
+ " {6, 7, 8}\n"
+ "};\n",
+ Style);
+ verifyFormat("struct test demo[] = {\n"
+ " {1, 2, 3, 4, 5},\n"
+ " {3, 4, 5},\n"
+ " {6, 7, 8, 9, 10, 11, 12}\n"
+ "};\n",
+ Style);
+ verifyFormat("struct test demo[] = {\n"
+ " {1, 2, 3},\n"
+ " {3, 4, 5},\n"
+ " {6, 7, 8, 9, 10, 11, 12}\n"
+ "};\n",
+ Style);
+
+ verifyFormat("S{\n"
+ " {},\n"
+ " {},\n"
+ " {a, b}\n"
+ "};\n",
+ Style);
+ verifyFormat("S{\n"
+ " {},\n"
+ " {},\n"
+ " {a, b},\n"
+ "};\n",
+ Style);
+ verifyFormat("void foo() {\n"
+ " auto thing = test{\n"
+ " {\n"
+ " {13}, {something}, // A\n"
+ " }\n"
+ " };\n"
+ "}",
+ "void foo() {\n"
+ " auto thing = test{\n"
+ " {\n"
+ " {13},\n"
+ " {something}, // A\n"
+ " }\n"
+ " };\n"
+ "}",
+ Style);
+}
+
+TEST_F(FormatTest, AlignArrayOfStructuresRightAlignmentNonSquare) {
+ auto Style = getLLVMStyle();
+ Style.AlignArrayOfStructures = FormatStyle::AIAS_Right;
+ Style.AlignConsecutiveAssignments =
+ FormatStyle::AlignConsecutiveStyle::ACS_Consecutive;
+ Style.AlignConsecutiveDeclarations =
+ FormatStyle::AlignConsecutiveStyle::ACS_Consecutive;
+
+ // The AlignArray code is incorrect for non square Arrays and can cause
+ // crashes, these tests assert that the array is not changed but will
+ // also act as regression tests for when it is properly fixed
+ verifyFormat("struct test demo[] = {\n"
+ " {1, 2},\n"
+ " {3, 4, 5},\n"
+ " {6, 7, 8}\n"
+ "};\n",
+ Style);
+ verifyFormat("struct test demo[] = {\n"
+ " {1, 2, 3, 4, 5},\n"
+ " {3, 4, 5},\n"
+ " {6, 7, 8}\n"
+ "};\n",
+ Style);
+ verifyFormat("struct test demo[] = {\n"
+ " {1, 2, 3, 4, 5},\n"
+ " {3, 4, 5},\n"
+ " {6, 7, 8, 9, 10, 11, 12}\n"
+ "};\n",
+ Style);
+ verifyFormat("struct test demo[] = {\n"
+ " {1, 2, 3},\n"
+ " {3, 4, 5},\n"
+ " {6, 7, 8, 9, 10, 11, 12}\n"
+ "};\n",
+ Style);
+
+ verifyFormat("S{\n"
+ " {},\n"
+ " {},\n"
+ " {a, b}\n"
+ "};\n",
+ Style);
+ verifyFormat("S{\n"
+ " {},\n"
+ " {},\n"
+ " {a, b},\n"
+ "};\n",
+ Style);
+ verifyFormat("void foo() {\n"
+ " auto thing = test{\n"
+ " {\n"
+ " {13}, {something}, // A\n"
+ " }\n"
+ " };\n"
+ "}",
+ "void foo() {\n"
+ " auto thing = test{\n"
+ " {\n"
+ " {13},\n"
+ " {something}, // A\n"
+ " }\n"
+ " };\n"
+ "}",
+ Style);
+}
+
} // namespace
} // namespace format
} // namespace clang
Index: clang/lib/Format/WhitespaceManager.h
===================================================================
--- clang/lib/Format/WhitespaceManager.h
+++ clang/lib/Format/WhitespaceManager.h
@@ -196,8 +196,21 @@
struct CellDescriptions {
SmallVector<CellDescription> Cells;
- unsigned CellCount = 0;
+ SmallVector<unsigned> CellCounts;
unsigned InitialSpaces = 0;
+
+ // Determine if every row in the the array
+ // has the same number of columns.
+ bool HasEqualRowLengths() const {
+ if (CellCounts.empty())
+ return false;
+
+ for (auto NumberOfColumns : CellCounts) {
+ if (NumberOfColumns != CellCounts[0])
+ return false;
+ }
+ return true;
+ }
};
/// Calculate \c IsTrailingComment, \c TokenLength for the last tokens
@@ -295,13 +308,16 @@
/// Get The maximum width of all columns to a given cell.
template <typename I>
unsigned getMaximumNetWidth(const I &CellStart, const I &CellStop,
- unsigned InitialSpaces,
- unsigned CellCount) const {
+ unsigned InitialSpaces, unsigned CellCount,
+ unsigned MaxRowCount) const {
auto MaxNetWidth = getNetWidth(CellStart, CellStop, InitialSpaces);
auto RowCount = 1U;
auto Offset = std::distance(CellStart, CellStop);
for (const auto *Next = CellStop->NextColumnElement; Next != nullptr;
Next = Next->NextColumnElement) {
+ if (RowCount > MaxRowCount) {
+ break;
+ }
auto Start = (CellStart + RowCount * CellCount);
auto End = Start + Offset;
MaxNetWidth =
Index: clang/lib/Format/WhitespaceManager.cpp
===================================================================
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -1032,11 +1032,13 @@
void WhitespaceManager::alignArrayInitializersRightJustified(
CellDescriptions &&CellDescs) {
- auto &Cells = CellDescs.Cells;
+ if (!CellDescs.HasEqualRowLengths())
+ return;
+ auto &Cells = CellDescs.Cells;
// Now go through and fixup the spaces.
auto *CellIter = Cells.begin();
- for (auto i = 0U; i < CellDescs.CellCount; ++i, ++CellIter) {
+ for (auto i = 0U; i < CellDescs.CellCounts[0]; ++i, ++CellIter) {
unsigned NetWidth = 0U;
if (isSplitCell(*CellIter))
NetWidth = getNetWidth(Cells.begin(), CellIter, CellDescs.InitialSpaces);
@@ -1058,16 +1060,17 @@
if (CellIter != Cells.begin()) {
auto ThisNetWidth =
getNetWidth(Cells.begin(), CellIter, CellDescs.InitialSpaces);
- auto MaxNetWidth =
- getMaximumNetWidth(Cells.begin(), CellIter, CellDescs.InitialSpaces,
- CellDescs.CellCount);
+ auto MaxNetWidth = getMaximumNetWidth(
+ Cells.begin(), CellIter, CellDescs.InitialSpaces,
+ CellDescs.CellCounts[0], CellDescs.CellCounts.size());
if (ThisNetWidth < MaxNetWidth)
Changes[CellIter->Index].Spaces = (MaxNetWidth - ThisNetWidth);
auto RowCount = 1U;
auto Offset = std::distance(Cells.begin(), CellIter);
for (const auto *Next = CellIter->NextColumnElement; Next != nullptr;
Next = Next->NextColumnElement) {
- auto *Start = (Cells.begin() + RowCount * CellDescs.CellCount);
+ auto *Start =
+ (Cells.begin() + RowCount * CellDescs.CellCounts[RowCount]);
auto *End = Start + Offset;
ThisNetWidth = getNetWidth(Start, End, CellDescs.InitialSpaces);
if (ThisNetWidth < MaxNetWidth)
@@ -1100,8 +1103,11 @@
void WhitespaceManager::alignArrayInitializersLeftJustified(
CellDescriptions &&CellDescs) {
- auto &Cells = CellDescs.Cells;
+ if (!CellDescs.HasEqualRowLengths())
+ return;
+
+ 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.
@@ -1110,9 +1116,10 @@
else
Changes[CellIter->Index].Spaces = CellDescs.InitialSpaces;
++CellIter;
- for (auto i = 1U; i < CellDescs.CellCount; i++, ++CellIter) {
+ for (auto i = 1U; i < CellDescs.CellCounts[0]; i++, ++CellIter) {
auto MaxNetWidth = getMaximumNetWidth(
- Cells.begin(), CellIter, CellDescs.InitialSpaces, CellDescs.CellCount);
+ 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) {
@@ -1124,7 +1131,10 @@
auto Offset = std::distance(Cells.begin(), CellIter);
for (const auto *Next = CellIter->NextColumnElement; Next != nullptr;
Next = Next->NextColumnElement) {
- auto *Start = (Cells.begin() + RowCount * CellDescs.CellCount);
+ 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) {
@@ -1152,7 +1162,7 @@
unsigned Depth = 0;
unsigned Cell = 0;
- unsigned CellCount = 0;
+ SmallVector<unsigned> CellCounts;
unsigned InitialSpaces = 0;
unsigned InitialTokenLength = 0;
unsigned EndSpaces = 0;
@@ -1192,7 +1202,8 @@
if (!Cells.empty())
Cells.back().EndIndex = i;
Cells.push_back(CellDescription{i, ++Cell, i + 1, false, nullptr});
- CellCount = C.Tok->Previous->isNot(tok::comma) ? Cell + 1 : Cell;
+ CellCounts.push_back(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))
@@ -1262,7 +1273,7 @@
}
}
- return linkCells({Cells, CellCount, InitialSpaces});
+ return linkCells({Cells, CellCounts, InitialSpaces});
}
unsigned WhitespaceManager::calculateCellWidth(unsigned Start, unsigned End,
Index: clang/include/clang/Format/Format.h
===================================================================
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -131,6 +131,10 @@
};
/// if not ``None``, when using initialization for an array of structs
/// aligns the fields into columns.
+ ///
+ /// NOTE: As of clang-format 15 this option only applied to arrays with equal
+ /// number of columns per row.
+ ///
/// \version 13
ArrayInitializerAlignmentStyle AlignArrayOfStructures;
Index: clang/docs/ClangFormatStyleOptions.rst
===================================================================
--- clang/docs/ClangFormatStyleOptions.rst
+++ clang/docs/ClangFormatStyleOptions.rst
@@ -236,6 +236,9 @@
if not ``None``, when using initialization for an array of structs
aligns the fields into columns.
+ NOTE: As of clang-format 15 this option only applied to arrays with equal
+ number of columns per row.
+
Possible values:
* ``AIAS_Left`` (in configuration: ``Left``)
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
