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

[Fred Grim via Phabricator via cfe-commits]

feg208 added a comment.

To answer the question of why I think this is different then other alignment options....It seems to me that each alignment option emphasizes a specific thing, be it macros, bitfields, or (maybe closer in spirit) more simple declarations and assignments. I think this case is unique and not currently addressed by any of the existing alignment mechanisms.



================
Comment at: clang/include/clang/Format/Format.h:93
 
+  /// if ``true``, when using static initialization for an array of structs
+  /// aligns the fields into columns
----------------
curdeius wrote:
> Why static?
I was quoting from an internal ticket but it doesn't make sense. I removed it


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:32
+// The notion here is that we walk through the annotated line looking for
+// things like static initialization of arrays and flag them
+bool isArrayOfStructuresInit(const AnnotatedLine &Line) {
----------------
HazardyKnusperkeks wrote:
> Dot at the end. :)
One of these days I'll learn the English language. Then watch out!


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:37
+  const auto *CurrentToken = Line.First;
+  enum class DetectAsiFsm {
+    null,
----------------
HazardyKnusperkeks wrote:
> What stands AsiFsm for?
Aligned Structure Initializer Finite State Machine but honestly this entire flow was misguided as the suggested tests pointed out. I switched it to something that fits (I think) a bit better with a wider array of possible inputs


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1232
+      } else if (CurrentToken->is(tok::r_brace)) {
+        Depth--;
+      }
----------------
HazardyKnusperkeks wrote:
> Why postfix, especially if using prefix in the `if`?
just an oversight. In hindsight it does look goofy ehh?


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1404
+                                         isArrayOfStructuresInit(TheLine));
+      if (AlignArrayOfStructuresInit) {
+        Penalty += AlignedArrayOfStructuresInitLineFormatter(
----------------
HazardyKnusperkeks wrote:
> I'm not sure that you should go before the no column limit. In either case you should have a test case for that.
I kept the ordering but I will admit to misgivings about it. None of the tests fail and it seems like it needs to be ordered in this way but as a newbie contributor to this project I don't feel confident in that assertion. Do the included tests cover it? What test that isn't currently there would better create issues?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101868