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

MyDeveloperDay added inline comments. Comment at: clang/include/clang/Format/Format.h:563 + }; + + TrailingCommaStyle InsertTrailingCommas; I think we need to add some text in here to ensure it gets documented automatically in ClangFormatStyleOptions.rst rathe

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

MyDeveloperDay added a comment. In D73354#1854115 , @mprobst wrote: > This has landed as https://reviews.llvm.org/rGa324fcf1ae6 (not sure why this > hasn't closed automatically). Thank you, as for not autoclosing my guess it because perhaps you dropped

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

mprobst closed this revision. mprobst marked an inline comment as done. mprobst added a comment. This has landed as https://reviews.llvm.org/rGa324fcf1ae6 (not sure why this hasn't closed automatically). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org

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

MyDeveloperDay added a comment. Nit: don't forget the ClangFormatStyleOption.rst and ReleaseNote.rst changes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73354/new/ https://reviews.llvm.org/D73354 ___

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

MyDeveloperDay accepted this revision. MyDeveloperDay added a comment. I'm happy if you are, I think given that you've really been one of the main contributors of clang-format [JS] for the last number of years you are the best person to probably assess the impact on Javascript code. Thanks for

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

mprobst marked 4 inline comments as done. mprobst added inline comments. Comment at: clang/include/clang/Format/Format.h:42 + Unsuitable, + BinBackTrailingCommaConflict +}; sammccall wrote: > Back->Pack? That's what you get when you fix my auto-complete: consis

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

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 u

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

mprobst marked 2 inline comments as done. mprobst added inline comments. Comment at: clang/lib/Format/Format.cpp:2531 +}); + auto Env = MyDeveloperDay wrote: > Ok, this comment is more a discussion point rather than a review comment. Was > there a reason

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

MyDeveloperDay added inline comments. Comment at: clang/lib/Format/Format.cpp:2531 +}); + auto Env = 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 F

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

mprobst marked 6 inline comments as done. mprobst added a comment. PTAL. Comment at: clang/include/clang/Format/Format.h:552 +/// Insert trailing commas in container literals that were wrapped over +/// multiple lines. +TCS_Wrapped, mprobst wrote: >

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

mprobst updated this revision to Diff 240873. mprobst marked an inline comment as done. mprobst added a comment. - - only run comma insertion for JavaScript. - review fixes - Fix col limit - test for comma insertion - - validate options, reject bin packing + trailing commas Repository: rG LLVM

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

MyDeveloperDay added a comment. Nit: Could you add an entry for ReleaseNotes.rst and regenerate the ClangFormatStyleOption.rst with the docs/tools/dump_style.py Comment at: clang/unittests/Format/FormatTestJS.cpp:1229 " (param): param is {\n" -

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

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: >

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

mprobst updated this revision to Diff 240847. mprobst marked 16 inline comments as done. mprobst added a comment. - - only run comma insertion for JavaScript. - review fixes - Fix col limit - test for comma insertion Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://revi

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

krasimir added inline comments. Comment at: clang/lib/Format/Format.cpp:1482 +/// ]; +class TrailingCommaInserter : public TokenAnalyzer { +public: sammccall wrote: > Worth a comment (somewhere) about the fact that this never rewraps, in > particular if the

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

MyDeveloperDay added a comment. FYI maybe related D33029: [clang-format] add option for dangling parenthesis Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73354/new/ https://reviews.llvm.org/D73354

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

krasimir added a comment. An alternative approach (I'm not endorsing this) that would *in theory* circumvent non-idempotency issue (but would be more fiddly~fiddly to implement and spill across different layers of the formatter): This is just a rough idea and can have multiple issues in itself.

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

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 ple

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

sammccall added a comment. In D73354#1838964 , @Bouska wrote: > I have an issue with this change. Currently (at least for C++), the presence > of a trailing comma is used as a formatting hint to put all the element in > one line or one per line as below:

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

Bouska added a comment. I have an issue with this change. Currently (at least for C++), the presence of a trailing comma is used as a formatting hint to put all the element in one line or one per line as below: enum test1 = {ONE, TWO, THREE}; enum test2 = { ONE, TWO, TH

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

Bouska added a comment. 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/mai

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

sammccall added a comment. For rolling this out, I think the best path is: - check in the option, but don't turn it on with any styles yet - test it by taking a large codebase, formatting it (normal options), format it (with comma insertion), look at the diffs (internally google3diff can do this

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

krasimir added a comment. General comment: I think this is useful and could also be useful for other languages too (some folks have asked for this for some C++ code). As this is a non-whitespace only change, it's a bit more risky than whitespace-only changes. I think we need to establish a pol

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

mprobst updated this revision to Diff 240194. mprobst added a comment. - - only run comma insertion for JavaScript. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73354/new/ https://reviews.llvm.org/D73354 Files: clang/include/clang/Format/Format

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

mprobst added a comment. This is required by JavaScript style, but this change as is optimistically runs the tool for any language. I think that's probably not what we want here initially, just throwing it out there. WDYT? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION http

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

mprobst created this revision. mprobst added reviewers: krasimir, MyDeveloperDay. Herald added a project: clang. This change adds an option to insert trailing commas into container literals. For example, in JavaScript: const x = [ a, b, ^ inserted if missing. ] This is imple