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

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

HazardyKnusperkeks added inline comments.


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1404
+                                         isArrayOfStructuresInit(TheLine));
+      if (AlignArrayOfStructuresInit) {
+        Penalty += AlignedArrayOfStructuresInitLineFormatter(
----------------
feg208 wrote:
> 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?
See below.


================
Comment at: clang/unittests/Format/FormatTest.cpp:16352
 
+template <size_t M, size_t N>
+auto createStylesImpl(
----------------
This is a nice approach!

Could it be made constexpr? I'm not sure about C++14.


================
Comment at: clang/unittests/Format/FormatTest.cpp:16371
+               Style);
+}
+
----------------
feg208 wrote:
> HazardyKnusperkeks wrote:
> > 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`?)
> I added
> 
>   - 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 multiple string literals) But I still should add another test I think
>   - with non-plain-array types (i.e. missing []/[3])
>   - interaction with other Align* options (e.g. AlignConsecutiveAssignments, two arrays one after another, AlignConsecutiveDeclarations). I will say that I did this in a brute force way. Maybe there is something else I should do here?
>   - With no column limit.
> 
> I still need tests with east and west const and without verifyFormat. For the latter is there an example test I could look at? Idly scrolling through FormatTest.cpp it seems like most stuff uses verifyFormat? Or maybe I misunderstood the request?
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
What's missing is to format your last code with a column limit of 20 or so.

Regards using `EXPECT_EQ`:
```
EXPECT_EQ("test demo[] = {\n"
      "    {56, 23,    \"hello world i am a very long line that really, in any "
      "\"\n"
      "                \"just world, ought be split over multiple lines\" },\n"
      "    {-1, 93463, \"world\"                                          },\n"
      "    {7,  5,     \"!!\"                                             },\n"
      "};\n",
format("test demo[] = {{56, 23, \"hello world i am a very long line that really, in any just world, ought be split over multiple lines\"},{-1, 93463, \"world\"},{7, 5, \"!!\"},};", Style));
```

This should be done with ColumnLimit 0, 20, 100.

With `verifyFormat` you verify that the formatting is stable, but especially with no column limit there is often no stable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101868