[PATCH] D32989: Don't indent JavaScript IIFEs

[Martin Probst via Phabricator via cfe-commits]

mprobst added inline comments.


================
Comment at: lib/Format/UnwrappedLineParser.cpp:2346
 
+bool UnwrappedLineParser::isIIFE() const {
+  // Look for the start of an immediately invoked anonymous function.
----------------
Why not just a static function?


================
Comment at: lib/Format/UnwrappedLineParser.cpp:2351
+  // Example: (function() { ... })()
+  // FIXME: check for the end invocation or alternate ways to start function
+  // expressions?
----------------
The question is whether we ever expect to find lines starting with `(function() {` that do not end up being an IIFE. I think that's very unlikely, so I think I'd even drop this FIXME. I wouldn't expect that to be something that needs fixing.

If you wanted to, you could check FormatToken.MatchingParen and then parse from there, but I'm not sure if that pointer is already set up at this point.


================
Comment at: lib/Format/UnwrappedLineParser.cpp:2353
+  // expressions?
+  if (Line->Tokens.size() < 5)
+    return false;
----------------
There's a `startsSequenceInternal` on `Line` that might come in handy here?


================
Comment at: unittests/Format/FormatTestJS.cpp:371
+TEST_F(FormatTestJS, IIFE) {
+  verifyFormat("(function() {\n"
+               "var a = 1;\n"
----------------
consider adding a test with comments between the tokens (which should work with `startsSequence`).


================
Comment at: unittests/Format/FormatTestJS.cpp:374
+               "}())");
+  verifyFormat("(function() {\n"
+               "var b = 2;\n"
----------------
I find it hard to see how these cases are different - can you give them e.g. variable names that call out what's the point of the different cases?


https://reviews.llvm.org/D32989