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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

(Maybe we should have a basic test for C++, but I'm not sure how this usually goes)

BTW it occurs to me that turning this on by default may be a surprising breakage for:

- people who use clang-format to format JSON, where the comma is illegal.
- people who care about IE8 or something



================
Comment at: clang/include/clang/Format/Format.h:42
+  Unsuitable,
+  BinBackTrailingCommaConflict
+};
----------------
Back->Pack?


================
Comment at: clang/lib/Format/Format.cpp:2531
+    });
+
   auto Env =
----------------
mprobst wrote:
> MyDeveloperDay wrote:
> > Ok, this comment is more a discussion point rather than a review comment. Was there a reason you needed to put the TrailingCommaInserter pass after the Formatter pass? (maybe you needed the Tokens annotated?)
> > 
> > From my understanding of some of the other conversations, it seemed the reason for ignoring the Column limit was because the "," insertion came after it had been formatted (is that correct?)
> > 
> > If there was a good reason that's also fine, I was just interested to learn if there was some part of the Formatter that perhaps needs to be pulled out into its own separate pass.
> > 
> > 
> The problem with inserting the comma during formatting is that it'd need to be inserted during the state exploration as part of Dijkstra's implemented in clang-format. I've considered that, but it'd be complex (if we make formatting decision X, we now add a token, which might invalidate the formatting decision). Keeping it as a separate pass has drawbacks, such as potentially not ending up with perfect formatting, thus the backing off to insert over ColumnLimit, but seems overall simpler.
> Ok, this comment is more a discussion point rather than a review comment. Was there a reason you needed to put the TrailingCommaInserter pass after the Formatter pass? (maybe you needed the Tokens annotated?)

The comma must only be inserted when the items are wrapped one-per-line (i.e. the last item is not on the same line as the closing bracket). This wrapping is a decision taken by the formatter, so we need to run it first to know.
(Apologies if this is obvious, but maybe missed)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73354