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

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

I really like alignment! :)

But why is this completely different than the other alignment options?

You should also add an entry to the release notes.



================
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) {
----------------
Dot at the end. :)


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:37
+  const auto *CurrentToken = Line.First;
+  enum class DetectAsiFsm {
+    null,
----------------
What stands AsiFsm for?


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:82
+    }
+    CurrentToken = CurrentToken->getNextNonComment();
+  }
----------------
I would prefer a
`for(; CurrentToken != Line.Last && CurrentToken != nullptr; CurrentToken = CurrentToken->getNextNonComment())


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1232
+      } else if (CurrentToken->is(tok::r_brace)) {
+        Depth--;
+      }
----------------
Why postfix, especially if using prefix in the `if`?


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1244
+          Column = 0;
+          Row++;
+        } else {
----------------
Same.


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1276
+    }
+    Layout.emplace_back(std::vector<unsigned>{});
+    auto Row = Layout.size() - 1;
----------------
With this you should save yourself the temporary.


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1278
+    auto Row = Layout.size() - 1;
+    Layout[Row].push_back(0);
+    auto Column = 0U;
----------------
But why not just initialize the vector directly with 1 element of 0?


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1316-1327
+    while (true) {
+      auto MaxColumn = 0U;
+      for (auto Row = 0U; Row < RowCount; Row++) {
+        MaxColumn = std::max(MaxColumn, LayoutMatrix[Row][Column]);
+      }
+      for (auto Row = 0U; Row < RowCount; Row++) {
+        LayoutMatrix[Row][Column] = MaxColumn - LayoutMatrix[Row][Column];
----------------
You should also assert that `LayoutMatrix.size() > 0`.


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1404
+                                         isArrayOfStructuresInit(TheLine));
+      if (AlignArrayOfStructuresInit) {
+        Penalty += AlignedArrayOfStructuresInitLineFormatter(
----------------
I'm not sure that you should go before the no column limit. In either case you should have a test case for that.


================
Comment at: clang/unittests/Format/FormatTest.cpp:16371
+               Style);
+}
+
----------------
curdeius wrote:
> I think we need more tests:
> * with a comma after the last element of array
> * with assignment and not only initialization?
> * without `struct` keyword
> * with short lines/long values to see how wrapping behaves
>   * also, with multiple string literals in one struct, e.g. `"hello" "world"` (that will get splitted into multiple lines)
> * with non-plain-array types (i.e. missing `[]`/`[3]`)
> * interaction with other Align* options (e.g. `AlignConsecutiveAssignments`, two arrays one after another, `AlignConsecutiveDeclarations`).
In addition to that I want to see:
* How it behaves with a smaller column limit which would be violated if there is no additional wrapping. There I also want one without `verifyFormat`.
* With no column limit.
* Maybe with a `const`, `inline` or something similar. (How about east `const` vs. west `const`?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101868