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

[Fred Grim via Phabricator via cfe-commits]

feg208 added a comment.

I still need to handle the 20 character case appropriately



================
Comment at: clang/unittests/Format/FormatTest.cpp:16465
+      "test demo[] = {\n"
+      "    {56, 23,    \"hello world i am a very long line that really, in any "
+      "just world, ought to be split over multiple lines\" },\n"
----------------
HazardyKnusperkeks wrote:
> But this line is longer than 20! This test should fail, shouldn't it?
It should. When I use the command line invocation I get different output

```
#include <array>
struct test {
  int a, b;
  const char *c;
};

int main(
    int,
    const char **) {
  std::array<
      struct test,
      3>
      demo;
  demo = std::array<
      struct test,
      3>{
      test{
          56, 23,
          "hello "
          "world i "
          "am a "
          "very "
          "long "
          "line "
          "that "
          "really, "
          "in any "
          "just "
          "world, "
          "ought "
          "to be "
          "split "
          "over "
          "multiple"
          " lines"},
      test{-1,
           93463,
           "world"},
      test{7, 5,
           "!!"},
  };
}
```
I will investigate


================
Comment at: clang/unittests/Format/FormatTest.cpp:16352
 
+template <size_t M, size_t N>
+auto createStylesImpl(
----------------
HazardyKnusperkeks wrote:
> feg208 wrote:
> > curdeius wrote:
> > > 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.
> > I think it could be constexpr (thats the direction I originally started which lead to much moaning about the lack of fold expressions) however getLLVMStyle() is not constexpr and it felt inappropriate for this commit to change the interface to that function even if the change were relatively harmless.
> > Also, when a test like this fails, it would be hard to see with which style it failed.
> 
> That is a valid point. You should only test some combinations.
I can certainly strip it down to corner cases. I guess I am not clear on which ones I should focus on. When I implemented this none of the combinations created problems. I could pick a few at random? But that doesn't feel like an improvement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101868