[PATCH] D32989: Don't indent JavaScript IIFEs
This revision was automatically updated to reflect the committed changes.
Closed by commit rL302580: clang-format: [JS] Don't indent JavaScript IIFEs.
(authored by mprobst).
Changed prior to commit:
https://reviews.llvm.org/D32989?vs=98332&id=98344#toc
Repository:
rL LLVM
https://reviews.llvm.org/D32989
Files:
cfe/trunk/lib/Format/UnwrappedLineParser.cpp
cfe/trunk/unittests/Format/FormatTestJS.cpp
Index: cfe/trunk/lib/Format/UnwrappedLineParser.cpp
===
--- cfe/trunk/lib/Format/UnwrappedLineParser.cpp
+++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp
@@ -476,6 +476,24 @@
return I->Tok->is(tok::l_paren);
}
+static bool isIIFE(const UnwrappedLine &Line,
+ const AdditionalKeywords &Keywords) {
+ // Look for the start of an immediately invoked anonymous function.
+ // https://en.wikipedia.org/wiki/Immediately-invoked_function_expression
+ // This is commonly done in JavaScript to create a new, anonymous scope.
+ // Example: (function() { ... })()
+ if (Line.Tokens.size() < 3)
+return false;
+ auto I = Line.Tokens.begin();
+ if (I->Tok->isNot(tok::l_paren))
+return false;
+ ++I;
+ if (I->Tok->isNot(Keywords.kw_function))
+return false;
+ ++I;
+ return I->Tok->is(tok::l_paren);
+}
+
static bool ShouldBreakBeforeBrace(const FormatStyle &Style,
const FormatToken &InitialToken) {
if (InitialToken.is(tok::kw_namespace))
@@ -493,15 +511,16 @@
FormatTok->BlockKind = BK_Block;
nextToken();
{
-bool GoogScope =
-Style.Language == FormatStyle::LK_JavaScript && isGoogScope(*Line);
+bool SkipIndent =
+(Style.Language == FormatStyle::LK_JavaScript &&
+ (isGoogScope(*Line) || isIIFE(*Line, Keywords)));
ScopedLineState LineState(*this);
ScopedDeclarationState DeclarationState(*Line, DeclarationScopeStack,
/*MustBeDeclaration=*/false);
-Line->Level += GoogScope ? 0 : 1;
+Line->Level += SkipIndent ? 0 : 1;
parseLevel(/*HasOpeningBrace=*/true);
flushComments(isOnNewLine(*FormatTok));
-Line->Level -= GoogScope ? 0 : 1;
+Line->Level -= SkipIndent ? 0 : 1;
}
nextToken();
}
Index: cfe/trunk/unittests/Format/FormatTestJS.cpp
===
--- cfe/trunk/unittests/Format/FormatTestJS.cpp
+++ cfe/trunk/unittests/Format/FormatTestJS.cpp
@@ -367,6 +367,25 @@
"});");
}
+TEST_F(FormatTestJS, IIFEs) {
+ // Internal calling parens; no semi.
+ verifyFormat("(function() {\n"
+ "var a = 1;\n"
+ "}())");
+ // External calling parens; no semi.
+ verifyFormat("(function() {\n"
+ "var b = 2;\n"
+ "})()");
+ // Internal calling parens; with semi.
+ verifyFormat("(function() {\n"
+ "var c = 3;\n"
+ "}());");
+ // External calling parens; with semi.
+ verifyFormat("(function() {\n"
+ "var d = 4;\n"
+ "})();");
+}
+
TEST_F(FormatTestJS, GoogModules) {
verifyFormat("goog.module('this.is.really.absurdly.long');",
getGoogleJSStyleWithColumns(40));
Index: cfe/trunk/lib/Format/UnwrappedLineParser.cpp
===
--- cfe/trunk/lib/Format/UnwrappedLineParser.cpp
+++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp
@@ -476,6 +476,24 @@
return I->Tok->is(tok::l_paren);
}
+static bool isIIFE(const UnwrappedLine &Line,
+ const AdditionalKeywords &Keywords) {
+ // Look for the start of an immediately invoked anonymous function.
+ // https://en.wikipedia.org/wiki/Immediately-invoked_function_expression
+ // This is commonly done in JavaScript to create a new, anonymous scope.
+ // Example: (function() { ... })()
+ if (Line.Tokens.size() < 3)
+return false;
+ auto I = Line.Tokens.begin();
+ if (I->Tok->isNot(tok::l_paren))
+return false;
+ ++I;
+ if (I->Tok->isNot(Keywords.kw_function))
+return false;
+ ++I;
+ return I->Tok->is(tok::l_paren);
+}
+
static bool ShouldBreakBeforeBrace(const FormatStyle &Style,
const FormatToken &InitialToken) {
if (InitialToken.is(tok::kw_namespace))
@@ -493,15 +511,16 @@
FormatTok->BlockKind = BK_Block;
nextToken();
{
-bool GoogScope =
-Style.Language == FormatStyle::LK_JavaScript && isGoogScope(*Line);
+bool SkipIndent =
+(Style.Language == FormatStyle::LK_JavaScript &&
+ (isGoogScope(*Line) || isIIFE(*Line, Keywords)));
ScopedLineState LineState(*this);
ScopedDeclarationState DeclarationState(*Line, DeclarationScopeStack,
/*MustBeDeclaration=*/false);
-Line->Level += GoogScope ? 0 : 1;
+Line->Level += SkipIndent ? 0 : 1;
parseLevel(/*HasOpeningBrace=*/true);
flushComments(isOnNewLine(*For
[PATCH] D32989: Don't indent JavaScript IIFEs
mprobst accepted this revision.
mprobst added inline comments.
This revision is now accepted and ready to land.
Comment at: unittests/Format/FormatTestJS.cpp:371
+TEST_F(FormatTestJS, IIFE) {
+ verifyFormat("(function() {\n"
+ "var a = 1;\n"
danbeam wrote:
> mprobst wrote:
> > consider adding a test with comments between the tokens (which should work
> > with `startsSequence`).
> not done yet, but seems uncommon.
>
> do you want me to add a FIXME?
I think that's OK the way it is.
https://reviews.llvm.org/D32989
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32989: Don't indent JavaScript IIFEs
danbeam added inline comments.
Comment at: lib/Format/UnwrappedLineParser.cpp:2353
+ // expressions?
+ if (Line->Tokens.size() < 5)
+return false;
mprobst wrote:
> There's a `startsSequenceInternal` on `Line` that might come in handy here?
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)
Comment at: lib/Format/UnwrappedLineParser.cpp:2362
+ ++I;
+ if (I->Tok->isNot(tok::l_paren))
+return false;
mprobst wrote:
> One more - do we want to support IIFEs that take arguments?
>
>
> ```
> (function(global) {
> ...
> }(window));
> ```
"Less is more"
Comment at: unittests/Format/FormatTestJS.cpp:371
+TEST_F(FormatTestJS, IIFE) {
+ verifyFormat("(function() {\n"
+ "var a = 1;\n"
mprobst wrote:
> consider adding a test with comments between the tokens (which should work
> with `startsSequence`).
not done yet, but seems uncommon.
do you want me to add a FIXME?
https://reviews.llvm.org/D32989
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32989: Don't indent JavaScript IIFEs
danbeam updated this revision to Diff 98332.
danbeam marked 4 inline comments as done.
danbeam added a comment.
mprobst@ review
https://reviews.llvm.org/D32989
Files:
lib/Format/UnwrappedLineParser.cpp
unittests/Format/FormatTestJS.cpp
Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -367,6 +367,25 @@
"});");
}
+TEST_F(FormatTestJS, IIFEs) {
+ // Internal calling parens; no semi.
+ verifyFormat("(function() {\n"
+ "var a = 1;\n"
+ "}())");
+ // External calling parens; no semi.
+ verifyFormat("(function() {\n"
+ "var b = 2;\n"
+ "})()");
+ // Internal calling parens; with semi.
+ verifyFormat("(function() {\n"
+ "var c = 3;\n"
+ "}());");
+ // External calling parens; with semi.
+ verifyFormat("(function() {\n"
+ "var d = 4;\n"
+ "})();");
+}
+
TEST_F(FormatTestJS, GoogModules) {
verifyFormat("goog.module('this.is.really.absurdly.long');",
getGoogleJSStyleWithColumns(40));
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -476,6 +476,24 @@
return I->Tok->is(tok::l_paren);
}
+static bool isIIFE(const UnwrappedLine &Line,
+ const AdditionalKeywords &Keywords) {
+ // Look for the start of an immediately invoked anonymous function.
+ // https://en.wikipedia.org/wiki/Immediately-invoked_function_expression
+ // This is commonly done in JavaScript to create a new, anonymous scope.
+ // Example: (function() { ... })()
+ if (Line.Tokens.size() < 3)
+return false;
+ auto I = Line.Tokens.begin();
+ if (I->Tok->isNot(tok::l_paren))
+return false;
+ ++I;
+ if (I->Tok->isNot(Keywords.kw_function))
+return false;
+ ++I;
+ return I->Tok->is(tok::l_paren);
+}
+
static bool ShouldBreakBeforeBrace(const FormatStyle &Style,
const FormatToken &InitialToken) {
if (InitialToken.is(tok::kw_namespace))
@@ -493,15 +511,16 @@
FormatTok->BlockKind = BK_Block;
nextToken();
{
-bool GoogScope =
-Style.Language == FormatStyle::LK_JavaScript && isGoogScope(*Line);
+bool SkipIndent =
+(Style.Language == FormatStyle::LK_JavaScript &&
+ (isGoogScope(*Line) || isIIFE(*Line, Keywords)));
ScopedLineState LineState(*this);
ScopedDeclarationState DeclarationState(*Line, DeclarationScopeStack,
/*MustBeDeclaration=*/false);
-Line->Level += GoogScope ? 0 : 1;
+Line->Level += SkipIndent ? 0 : 1;
parseLevel(/*HasOpeningBrace=*/true);
flushComments(isOnNewLine(*FormatTok));
-Line->Level -= GoogScope ? 0 : 1;
+Line->Level -= SkipIndent ? 0 : 1;
}
nextToken();
}
Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -367,6 +367,25 @@
"});");
}
+TEST_F(FormatTestJS, IIFEs) {
+ // Internal calling parens; no semi.
+ verifyFormat("(function() {\n"
+ "var a = 1;\n"
+ "}())");
+ // External calling parens; no semi.
+ verifyFormat("(function() {\n"
+ "var b = 2;\n"
+ "})()");
+ // Internal calling parens; with semi.
+ verifyFormat("(function() {\n"
+ "var c = 3;\n"
+ "}());");
+ // External calling parens; with semi.
+ verifyFormat("(function() {\n"
+ "var d = 4;\n"
+ "})();");
+}
+
TEST_F(FormatTestJS, GoogModules) {
verifyFormat("goog.module('this.is.really.absurdly.long');",
getGoogleJSStyleWithColumns(40));
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -476,6 +476,24 @@
return I->Tok->is(tok::l_paren);
}
+static bool isIIFE(const UnwrappedLine &Line,
+ const AdditionalKeywords &Keywords) {
+ // Look for the start of an immediately invoked anonymous function.
+ // https://en.wikipedia.org/wiki/Immediately-invoked_function_expression
+ // This is commonly done in JavaScript to create a new, anonymous scope.
+ // Example: (function() { ... })()
+ if (Line.Tokens.size() < 3)
+return false;
+ auto I = Line.Tokens.begin();
+ if (I->Tok->isNot(tok::l_paren))
+return false;
+ ++I;
+ if (I->Tok->isNot(Keywords.kw_function))
+return false;
+ ++I;
+ return I->Tok->is(tok::l_paren);
+}
+
static bool ShouldBreakBeforeBrace(const FormatStyle &Style,
const FormatToken &InitialToken) {
if (InitialToken.
[PATCH] D32989: Don't indent JavaScript IIFEs
mprobst added inline comments.
Comment at: lib/Format/UnwrappedLineParser.cpp:2362
+ ++I;
+ if (I->Tok->isNot(tok::l_paren))
+return false;
One more - do we want to support IIFEs that take arguments?
```
(function(global) {
...
}(window));
```
https://reviews.llvm.org/D32989
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32989: Don't indent JavaScript IIFEs
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32989: Don't indent JavaScript IIFEs
danbeam created this revision.
Herald added a subscriber: klimek.
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
https://reviews.llvm.org/D32989
Files:
lib/Format/UnwrappedLineParser.cpp
lib/Format/UnwrappedLineParser.h
unittests/Format/FormatTestJS.cpp
Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -367,6 +367,21 @@
"});");
}
+TEST_F(FormatTestJS, IIFE) {
+ verifyFormat("(function() {\n"
+ "var a = 1;\n"
+ "}())");
+ verifyFormat("(function() {\n"
+ "var b = 2;\n"
+ "})()");
+ verifyFormat("(function() {\n"
+ "var c = 3;\n"
+ "})();");
+ verifyFormat("(function() {\n"
+ "var d = 4;\n"
+ "}());");
+}
+
TEST_F(FormatTestJS, GoogModules) {
verifyFormat("goog.module('this.is.really.absurdly.long');",
getGoogleJSStyleWithColumns(40));
Index: lib/Format/UnwrappedLineParser.h
===
--- lib/Format/UnwrappedLineParser.h
+++ lib/Format/UnwrappedLineParser.h
@@ -125,6 +125,7 @@
void nextToken();
const FormatToken *getPreviousToken();
void readToken();
+ bool isIIFE() const;
// Decides which comment tokens should be added to the current line and which
// should be added as comments before the next token.
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -493,15 +493,16 @@
FormatTok->BlockKind = BK_Block;
nextToken();
{
-bool GoogScope =
-Style.Language == FormatStyle::LK_JavaScript && isGoogScope(*Line);
+bool SkipIndent =
+Style.Language == FormatStyle::LK_JavaScript &&
+(isGoogScope(*Line) || isIIFE());
ScopedLineState LineState(*this);
ScopedDeclarationState DeclarationState(*Line, DeclarationScopeStack,
/*MustBeDeclaration=*/false);
-Line->Level += GoogScope ? 0 : 1;
+Line->Level += SkipIndent ? 0 : 1;
parseLevel(/*HasOpeningBrace=*/true);
flushComments(isOnNewLine(*FormatTok));
-Line->Level -= GoogScope ? 0 : 1;
+Line->Level -= SkipIndent ? 0 : 1;
}
nextToken();
}
@@ -2342,5 +2343,30 @@
}
}
+bool UnwrappedLineParser::isIIFE() const {
+ // Look for the start of an immediately invoked anonymous function.
+ // https://en.wikipedia.org/wiki/Immediately-invoked_function_expression
+ // This is commonly done in JavaScript to create a new, anonymous scope.
+ // Example: (function() { ... })()
+ // FIXME: check for the end invocation or alternate ways to start function
+ // expressions?
+ if (Line->Tokens.size() < 5)
+return false;
+ auto I = Line->Tokens.begin();
+ if (I->Tok->isNot(tok::l_paren))
+return false;
+ ++I;
+ if (I->Tok->isNot(Keywords.kw_function))
+return false;
+ ++I;
+ if (I->Tok->isNot(tok::l_paren))
+return false;
+ ++I;
+ if (I->Tok->isNot(tok::r_paren))
+return false;
+ ++I;
+ return I->Tok->is(tok::l_brace);
+}
+
} // end namespace format
} // end namespace clang
Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -367,6 +367,21 @@
"});");
}
+TEST_F(FormatTestJS, IIFE) {
+ verifyFormat("(function() {\n"
+ "var a = 1;\n"
+ "}())");
+ verifyFormat("(function() {\n"
+ "var b = 2;\n"
+ "})()");
+ verifyFormat("(function() {\n"
+ "var c = 3;\n"
+ "})();");
+ verifyFormat("(function() {\n"
+ "var d = 4;\n"
+ "}());");
+}
+
TEST_F(FormatTestJS, GoogModules) {
verifyFormat("goog.module('this.is.really.absurdly.long');",
getGoogleJSStyleWithColumns(40));
Index: lib/Format/UnwrappedLineParser.h
===
--- lib/Format/UnwrappedLineParser.h
+++ lib/Format/UnwrappedLineParser.h
@@ -125,6 +125,7 @@
void nextToken();
const FormatToken *getPreviousToken();
