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

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.


================
Comment at: clang/docs/ReleaseNotes.rst:247-248
 
+- Option ``AlignArrayOfStructure`` has been added to allow for ordering array-like
+  initializers into nice orderly columns while respecting column limits.
+
----------------



================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:920-923
+  auto getColumnLimitStyleAttribute() const noexcept {
+    return Style.ColumnLimit;
+  }
+
----------------
Having this in a function seems like a bad idea, it just makes the code less readable. Please inline.


================
Comment at: clang/unittests/Format/FormatTest.cpp:16352
 
+template <size_t M, size_t N>
+auto createStylesImpl(
----------------
HazardyKnusperkeks wrote:
> This is a nice approach!
> 
> Could it be made constexpr? I'm not sure about C++14.
Personally, I'm not really fond of testing all the combinations of styles. It adds little value to those tests.
I find it interesting to find problematic edge cases (which then should be added to the test suite "manually").
Also, when a test like this fails, it would be hard to see with which style it failed.


================
Comment at: clang/unittests/Format/FormatTest.cpp:16425
+                 Style);
+  }
+
----------------
Please add a test with preprocessor directives, e.g.:
```
    verifyFormat("struct test demo[] = {\n"
                 "    {56, 23,    \"hello\" },\n"
                 "#if X\n"
                 "    {-1, 93463, \"world\" },\n"
                 "#endif\n"
                 "    {7,  5,     \"!!\"    }\n"
                 "};\n",
                 Style);
```


================
Comment at: clang/unittests/Format/FormatTest.cpp:16433
+      "\"\n"
+      "                \"just world, ought be split over multiple lines\" },\n"
+      "    {-1, 93463, \"world\"                                          },\n"
----------------
Nit: "ought **to** be" :).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101868