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) - turn it on for -style=google This could all be in one commit, but that means coupling review of the code to getting results. ================ Comment at: clang/include/clang/Format/Format.h:552 + /// Insert trailing commas in container literals that were wrapped over + /// multiple lines. + TCS_Wrapped, ---------------- Hadn't occurred to me that this will interact with bin-packing. That seems to increase the chances of going over the column limit :( ================ Comment at: clang/lib/Format/Format.cpp:1477 +/// TrailingCommaInserter inserts trailing commas into container literals. +/// E.g.: ---------------- Inlining this in format.cpp seems worse than having passes in their own files, but is the pattern so far. Ugh, up to you. ================ Comment at: clang/lib/Format/Format.cpp:1482 +/// ]; +class TrailingCommaInserter : public TokenAnalyzer { +public: ---------------- 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. ================ Comment at: clang/lib/Format/Format.cpp:1511 + FormatToken *Matching = FormatTok->MatchingParen; + if (!Matching || !FormatTok->Previous) + continue; ---------------- 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) ================ Comment at: clang/lib/Format/Format.cpp:1526 + tooling::Replacement(Env.getSourceManager(), Start, 0, ",")); + // FIXME: handle error. For now, print error message and skip the + // replacement for release version. ---------------- this is a can't-happen case, right? (comma-insertions can't conflict with each other, and this pass gets its own Replacements). use cantFail(Result.add(...)) ================ Comment at: clang/unittests/Format/FormatTestJS.cpp:343 " for: 'for',\n" - " x: 'x'\n" + " x: 'x',\n" "};", ---------------- 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. ================ Comment at: clang/unittests/Format/FormatTestJS.cpp:1801 "}"); verifyFormat("export default [\n" + " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,\n" ---------------- Pretty hard to spot the change here, add a comment or change the a/bs to a description? I'd also place this next to a test that verifies that when you put the same construct on one line, no comma is inserted (even if the test is redundant), and include a case with a dict/map/hash/thing :-) 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