[PATCH] D121069: [clang-format] Minimize the damage caused by AlignArrayOfStructures when working on non square arrays

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.


================
Comment at: clang/lib/Format/WhitespaceManager.cpp:1073
+          auto *Start =
+              (Cells.begin() + RowCount * CellDescs.CellCounts[RowCount]);
           auto *End = Start + Offset;
----------------
Can we use `CellCounts[0]` instead? The outer parentheses are superfluous. Same in `alignArrayInitializersLeftJustified` below.


================
Comment at: clang/lib/Format/WhitespaceManager.cpp:1118
     Changes[CellIter->Index].Spaces = CellDescs.InitialSpaces;
   ++CellIter;
+  for (auto i = 1U; i < CellDescs.CellCounts[0]; i++, ++CellIter) {
----------------
MyDeveloperDay wrote:
> I don't understand in Left alignment why we ignore the first cell, but in right alignment, we don't!
Because the first cell is already left-aligned by default?


================
Comment at: clang/lib/Format/WhitespaceManager.h:204
+    // has the same number of columns.
+    bool HasEqualRowLengths() const {
+      if (CellCounts.empty())
----------------
My top choice by far is `isRectangular`.


================
Comment at: clang/lib/Format/WhitespaceManager.h:211
+          return false;
+      }
+      return true;
----------------
We can omit the braces here.


================
Comment at: clang/lib/Format/WhitespaceManager.h:318
          Next = Next->NextColumnElement) {
+      if (RowCount > MaxRowCount) {
+        break;
----------------
curdeius wrote:
> You can elide braces. Personally I use RemoveBraces locally.
Why do we need this check? Is it because `CellStop` might not be null terminated? Or is it because `CellCount` may be zero? If the latter, we can check `CellCount` before the loop instead of this check in the loop.


================
Comment at: clang/unittests/Format/FormatTest.cpp:25316
+               "    {6, 7, 8}\n"
+               "};\n",
+               Style);
----------------
Please remove `\n` here and in other places below when it's the last line of a test case.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121069/new/

https://reviews.llvm.org/D121069