[PATCH] D101868: [clang-format] Adds a formatter for aligning arrays of structs

[Marek Kurdej via Phabricator via cfe-commits]

curdeius accepted this revision.
curdeius added a comment.
This revision is now accepted and ready to land.

LGTM. That's a great piece work @feg208. Thank you!

I've added many nit comments, but I didn't do it for all code comments.
Please check that all comments are full phrases (with full stops :) ) before landing.
Some comments are in .rst but you know that you need to update Format.h and then regenerate .rst :).
Also, don't hesitate to mark comments as done.

Do you need somebody to land it on your behalf?



================
Comment at: clang/docs/ClangFormatStyleOptions.rst:213-218
+    struct test demo[] =
+    {
+        {56,    23, "hello"},
+        {-1, 93463, "world"},
+        { 7,     5,    "!!"}
+    }
----------------
This repetition of `demo` struct seems not very useful.


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:247
+  * ``AIAS_None`` (in configuration: ``None``)
+    Don't align array initializer columns
+
----------------
Nit. This can be done when landing. No need to update the patch just for this.


================
Comment at: clang/include/clang/Format/Format.h:93
 
+  /// Different style for aligning array initializers
+  enum ArrayInitializerAlignmentStyle {
----------------



================
Comment at: clang/lib/Format/FormatToken.h:434-442
+  /// The first token in set of column elements
+  bool StartsColumn = false;
+
+  /// This notes the start of the line of an array initializer
+  bool ArrayInitializerLineStart = false;
+
+  /// This starts an array initializer
----------------
Nits.


================
Comment at: clang/lib/Format/WhitespaceManager.cpp:994
+
+  // Now go through and fixup the spaces
+  auto *CellIter = Cells.begin();
----------------



================
Comment at: clang/lib/Format/WhitespaceManager.cpp:1005
+      // on a line that was split. If so on that line we make sure that
+      // the spaces in front of the brace are enough
+      Changes[CellIter->Index].NewlinesBefore = 0;
----------------



================
Comment at: clang/lib/Format/WhitespaceManager.cpp:1013-1014
+      }
+      // Except if the array is empty we need the position of all the
+      // cells immediantly adjacent to this
+      if (CellIter != Cells.begin()) {
----------------



================
Comment at: clang/lib/Format/WhitespaceManager.cpp:1062
+
+  // Now go through and fixup the spaces
+  auto *CellIter = Cells.begin();
----------------



================
Comment at: clang/lib/Format/WhitespaceManager.cpp:1064
+  auto *CellIter = Cells.begin();
+  // The first cell needs to be against the left brace
+  if (Changes[CellIter->Index].NewlinesBefore == 0)
----------------



================
Comment at: clang/lib/Format/WhitespaceManager.h:185-190
+    constexpr bool operator==(const CellDescription &oth) const {
+      return Index == oth.Index && Cell == oth.Cell && EndIndex == oth.EndIndex;
+    }
+    constexpr bool operator!=(const CellDescription &oth) const {
+      return !(*this == oth);
+    }
----------------
Nit.


================
Comment at: clang/lib/Format/WhitespaceManager.h:265
+      // If we broke the line the initial spaces are already
+      // accounted for
+      if (Changes[PrevIter->Index].NewlinesBefore > 0)
----------------
Nit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101868