[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
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();
[PATCH] D28165: Change clang-format's Chromium JavaScript defaults
danbeam updated this revision to Diff 82989.
danbeam marked an inline comment as done.
danbeam added a comment.
setting up arc
https://reviews.llvm.org/D28165
Files:
lib/Format/Format.cpp
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -638,6 +638,9 @@
ChromiumStyle.BreakAfterJavaFieldAnnotations = true;
ChromiumStyle.ContinuationIndentWidth = 8;
ChromiumStyle.IndentWidth = 4;
+ } else if (Language == FormatStyle::LK_JavaScript) {
+ChromiumStyle.AllowShortIfStatementsOnASingleLine = false;
+ChromiumStyle.AllowShortLoopsOnASingleLine = false;
} else {
ChromiumStyle.AllowAllParametersOfDeclarationOnNextLine = false;
ChromiumStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -638,6 +638,9 @@
ChromiumStyle.BreakAfterJavaFieldAnnotations = true;
ChromiumStyle.ContinuationIndentWidth = 8;
ChromiumStyle.IndentWidth = 4;
+ } else if (Language == FormatStyle::LK_JavaScript) {
+ChromiumStyle.AllowShortIfStatementsOnASingleLine = false;
+ChromiumStyle.AllowShortLoopsOnASingleLine = false;
} else {
ChromiumStyle.AllowAllParametersOfDeclarationOnNextLine = false;
ChromiumStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28165: Change clang-format's Chromium JavaScript defaults
danbeam marked an inline comment as done.
danbeam added inline comments.
Comment at: lib/Format/Format.cpp:643
+ChromiumStyle.AllowShortIfStatementsOnASingleLine = false;
+ChromiumStyle.AllowShortLoopsOnASingleLine = false;
+ }
thakis wrote:
> Thanks for the patch! Do we want these as false in Chromium's JS? I would've
> thought the diff would just be
>
> ```
> - } else {
> + } else if (Language != FormatStyle::LK_JavaScript)
> ```
>
> so that we just use google style for JS.
>
> If we do want to deviate from google style here for some reason then
> a) say why somewhere
> b) change the check for cpp to also include `|| Language ==
> FormatStyle::LK_ObjC`
>
> (If you include more diff context as described on
> http://llvm.org/docs/Phabricator.html, reviewing on phab is a bit easier.)
> so that we just use google style for JS
we want to tweak Google style a little bit. I mentioned this on the [[
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/OTEfsPvp0qc/UsaOTR9IEQAJ
| chromium-dev@ thread ]].
I added a specific branch for JS with explicit specializations so it's clearer
to the reader how Google and Chromium differ.
> (If you include more diff context as described on
> http://llvm.org/docs/Phabricator.html, reviewing on phab is a bit easier.)
Done.
https://reviews.llvm.org/D28165
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28165: Change clang-format's Chromium JavaScript defaults
danbeam updated this revision to Diff 82946.
danbeam added a comment.
make a branch specific to JS and duplicate some format options for explicitness
https://reviews.llvm.org/D28165
Files:
lib/Format/Format.cpp
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -638,6 +638,9 @@
ChromiumStyle.BreakAfterJavaFieldAnnotations = true;
ChromiumStyle.ContinuationIndentWidth = 8;
ChromiumStyle.IndentWidth = 4;
+ } else if (Language == FormatStyle::LK_JavaScript) {
+ChromiumStyle.AllowShortIfStatementsOnASingleLine = false;
+ChromiumStyle.AllowShortLoopsOnASingleLine = false;
} else {
ChromiumStyle.AllowAllParametersOfDeclarationOnNextLine = false;
ChromiumStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -638,6 +638,9 @@
ChromiumStyle.BreakAfterJavaFieldAnnotations = true;
ChromiumStyle.ContinuationIndentWidth = 8;
ChromiumStyle.IndentWidth = 4;
+ } else if (Language == FormatStyle::LK_JavaScript) {
+ChromiumStyle.AllowShortIfStatementsOnASingleLine = false;
+ChromiumStyle.AllowShortLoopsOnASingleLine = false;
} else {
ChromiumStyle.AllowAllParametersOfDeclarationOnNextLine = false;
ChromiumStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28165: Change clang-format's Chromium JavaScript defaults
danbeam created this revision.
danbeam added a reviewer: thakis.
danbeam added a subscriber: cfe-commits.
Herald added a subscriber: klimek.
Chromium is starting to use clang-format on more JavaScript.
In doing this, we discovered that our defaults were not doing a good job
differentiating between JS and C++.
This change moves some defaults to only apply to C++.
https://reviews.llvm.org/D28165
Files:
lib/Format/Format.cpp
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -639,13 +639,17 @@
ChromiumStyle.ContinuationIndentWidth = 8;
ChromiumStyle.IndentWidth = 4;
} else {
+ChromiumStyle.AllowShortIfStatementsOnASingleLine = false;
+ChromiumStyle.AllowShortLoopsOnASingleLine = false;
+ }
+
+ if (Language == FormatStyle::LK_Cpp) {
ChromiumStyle.AllowAllParametersOfDeclarationOnNextLine = false;
ChromiumStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
-ChromiumStyle.AllowShortIfStatementsOnASingleLine = false;
-ChromiumStyle.AllowShortLoopsOnASingleLine = false;
ChromiumStyle.BinPackParameters = false;
ChromiumStyle.DerivePointerAlignment = false;
}
+
ChromiumStyle.SortIncludes = false;
return ChromiumStyle;
}
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -639,13 +639,17 @@
ChromiumStyle.ContinuationIndentWidth = 8;
ChromiumStyle.IndentWidth = 4;
} else {
+ChromiumStyle.AllowShortIfStatementsOnASingleLine = false;
+ChromiumStyle.AllowShortLoopsOnASingleLine = false;
+ }
+
+ if (Language == FormatStyle::LK_Cpp) {
ChromiumStyle.AllowAllParametersOfDeclarationOnNextLine = false;
ChromiumStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
-ChromiumStyle.AllowShortIfStatementsOnASingleLine = false;
-ChromiumStyle.AllowShortLoopsOnASingleLine = false;
ChromiumStyle.BinPackParameters = false;
ChromiumStyle.DerivePointerAlignment = false;
}
+
ChromiumStyle.SortIncludes = false;
return ChromiumStyle;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
