This is an archive of the discontinued LLVM Phabricator instance.

Don't indent JavaScript IIFEs
ClosedPublic

Authored by danbeam on May 8 2017, 6:39 PM.

Details

Summary

Because IIFEs[1] are often used like an anonymous namespace around large
sections of JavaScript code, it's useful not to indent to them (which
effectively reduces the column limit by the indent amount needlessly).

It's also common for developers to wrap these around entire files or
libraries. When adopting clang-format, changing the indent entire file
can reduce the usefulness of the blame annotations.

Before:

(function() {
  // clang-format pushes stuff to here
})();

After:

(function() {
// clang-format pushes stuff to here
})();

[1] https://en.wikipedia.org/wiki/Immediately-invoked_function_expression

Diff Detail

Repository
rL LLVM

Event Timeline

danbeam created this revision.May 8 2017, 6:39 PM
danbeam edited the summary of this revision. (Show Details)May 8 2017, 6:40 PM
mprobst added inline comments.May 9 2017, 6:16 AM
lib/Format/UnwrappedLineParser.cpp
2346 ↗(On Diff #98243)

Why not just a static function?

2351 ↗(On Diff #98243)

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.

2353 ↗(On Diff #98243)

There's a startsSequenceInternal on Line that might come in handy here?

unittests/Format/FormatTestJS.cpp
371 ↗(On Diff #98243)

consider adding a test with comments between the tokens (which should work with startsSequence).

374 ↗(On Diff #98243)

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?

mprobst added inline comments.May 9 2017, 6:17 AM
lib/Format/UnwrappedLineParser.cpp
2362 ↗(On Diff #98243)

One more - do we want to support IIFEs that take arguments?

(function(global) {
  ...
}(window));
danbeam updated this revision to Diff 98332.May 9 2017, 11:48 AM
danbeam marked 4 inline comments as done.

mprobst@ review

lib/Format/UnwrappedLineParser.cpp
2353 ↗(On Diff #98243)

it wasn't on Line and unfortunately some of the state that startsSequence relies on isn't set up yet (at least as far as I know, I'm in a foreign land)

2362 ↗(On Diff #98243)

"Less is more"

unittests/Format/FormatTestJS.cpp
371 ↗(On Diff #98243)

not done yet, but seems uncommon.

do you want me to add a FIXME?

mprobst accepted this revision.May 9 2017, 1:14 PM
mprobst added inline comments.
unittests/Format/FormatTestJS.cpp
371 ↗(On Diff #98243)

I think that's OK the way it is.

This revision is now accepted and ready to land.May 9 2017, 1:14 PM
This revision was automatically updated to reflect the committed changes.