[PATCH] D73354: clang-format: insert trailing commas into containers.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I'm a little uncomfortable about all the tests changing, and I'm unsure if we should be changing the default behaviour.

The rules in the past here was to show that this is part of a public style guide. The assumption here is google style wants this. Could you please point to that documentation so at least there is some comeback when we break the world.



================
Comment at: clang/lib/Format/Format.cpp:956
     GoogleStyle.CommentPragmas = "(taze:|^/[ \t]*<|tslint:|@see)";
+    GoogleStyle.InsertTrailingCommas = FormatStyle::TCS_Wrapped;
     GoogleStyle.MaxEmptyLinesToKeep = 3;
----------------
Are you sure the right decision is to make this on by default for something that's going to insert the comma? is this in google's javascript style guide?

I think this could cause clang-format to suddenly start adding lost of commas (as we see  with the tests)


================
Comment at: clang/lib/Format/Format.cpp:1477
 
+/// TrailingCommaInserter inserts trailing commas into container literals.
+/// E.g.:
----------------
sammccall wrote:
> Inlining this in format.cpp seems worse than having passes in their own files, but is the pattern so far. Ugh, up to you.
Actually I think there is precedent to put TokenAnalyzers in their own class, I don't think it should be in Format.cpp


================
Comment at: clang/unittests/Format/FormatTestJS.cpp:1229
                "      (param): param is {\n"
-               "        a: SomeType\n"
+               "        a: SomeType;\n"
                "      }&ABC => 1)\n"
----------------
is this correct?


================
Comment at: clang/unittests/Format/FormatTestJS.cpp:1701
   verifyFormat("type X = {\n"
-               "  y: number\n"
+               "  y: number;\n"
                "};\n"
----------------
is this correct?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73354