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

[Martin Probst via Phabricator via cfe-commits]

mprobst marked 3 inline comments as done.
mprobst added inline comments.


================
Comment at: clang/include/clang/Format/Format.h:552
+    /// Insert trailing commas in container literals that were wrapped over
+    /// multiple lines.
+    TCS_Wrapped,
----------------
sammccall wrote:
> Hadn't occurred to me that this will interact with bin-packing. That seems to increase the chances of going over the column limit :(
Uh. Turns out we don't have a separate option for bin packing containers, only for bin packing arguments and parameters.

Should we add that option to make this setting work?
Should we have this setting imply bin packing containers (as opposed to parameters) is disabled?


================
Comment at: clang/lib/Format/Format.cpp:956
     GoogleStyle.CommentPragmas = "(taze:|^/[ \t]*<|tslint:|@see)";
+    GoogleStyle.InsertTrailingCommas = FormatStyle::TCS_Wrapped;
     GoogleStyle.MaxEmptyLinesToKeep = 3;
----------------
MyDeveloperDay wrote:
> 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)
It matches the style guide: https://google.github.io/styleguide/jsguide.html#features-arrays-trailing-comma

But it's indeed not entirely clear re bin packing, and certainly a good idea to split adding the option vs landing it.


================
Comment at: clang/lib/Format/Format.cpp:1477
 
+/// TrailingCommaInserter inserts trailing commas into container literals.
+/// E.g.:
----------------
MyDeveloperDay wrote:
> 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
Re inlining vs separate file, idk. On one hand, this file is on the large side, otoh these ~60 LOC don't carry their own weight in a separate file. Maybe in a follow up we should extract all passes into a separate file (maybe all passes except the actual indenter?).


================
Comment at: clang/lib/Format/Format.cpp:1482
+///     ];
+class TrailingCommaInserter : public TokenAnalyzer {
+public:
----------------
krasimir wrote:
> sammccall wrote:
> > Worth a comment (somewhere) about the fact that this never rewraps, in particular if the line is long, for simplicity.
> > 
> > Another policy that occurred to me: don't insert the comma if you're exactly at the line limit.
> +1 for "Another policy that occurred to me: don't insert the comma if you're exactly at the line limit." (or //over// the column limit). Seems like that would be enough to keep it idempotent.
Done re comment, and also done re policy, good idea.

I've also added a comment that this doesn't work together with bin packing.


================
Comment at: clang/lib/Format/Format.cpp:1511
+        FormatToken *Matching = FormatTok->MatchingParen;
+        if (!Matching || !FormatTok->Previous)
+          continue;
----------------
sammccall wrote:
> You're checking for the existence of a previous token, and below you use getPreviousNonComment() without a null-check.
> 
> I think you want to either assume that the existence of a match means there's a prev token (and remove the check here), or you want to verify in which case I think you also need to guard against it being a comment (by checking Prev below instead)
Good catch. I think the right fix is to check if there is a previous non comment token, as that's what I'm using below.


================
Comment at: clang/unittests/Format/FormatTestJS.cpp:343
                "  for: 'for',\n"
-               "  x: 'x'\n"
+               "  x: 'x',\n"
                "};",
----------------
sammccall wrote:
> AFAICT one-arg verifyFormat doesn't actually test this change - the new tests with commas should pass both before/after this change. 
> So there's just one testcase, I think.
Ha, I just adjusted the test cases to my change, but forgot uploading the patch with the tests.

I've reverted all these changes (because I'm splitting flipping the default from this), but added an explicit test.


================
Comment at: clang/unittests/Format/FormatTestJS.cpp:1229
                "      (param): param is {\n"
-               "        a: SomeType\n"
+               "        a: SomeType;\n"
                "      }&ABC => 1)\n"
----------------
MyDeveloperDay wrote:
> is this correct?
Yes, type definitions should use `;`


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73354