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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
