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

[Sam McCall via Phabricator via cfe-commits]

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