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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/include/clang/Format/Format.h:563
+  };
+
+  TrailingCommaStyle InsertTrailingCommas;

I think we need to add some text in here to ensure it gets documented 
automatically in ClangFormatStyleOptions.rst rather than the by hand version 
done in {D73768} otherwise revisions like {D78982} will end up removing it..


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D73354#1854115 , @mprobst wrote:

> This has landed as https://reviews.llvm.org/rGa324fcf1ae6 (not sure why this 
> hasn't closed automatically).


Thank you,

as for not autoclosing my guess it because perhaps you dropped the 
"Differential Revision" line from the commit message. I know there was some 
discussion about people not keeping all the Phabricator tags (Subscribers 
etc..), but I think if the commit message doesn't have the Differential 
Revision you won't get the AutoClose behaviour


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Probst via Phabricator via cfe-commits]

mprobst closed this revision.
mprobst marked an inline comment as done.
mprobst added a comment.

This has landed as https://reviews.llvm.org/rGa324fcf1ae6 (not sure why this 
hasn't closed automatically).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Nit: don't forget the ClangFormatStyleOption.rst and ReleaseNote.rst changes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay accepted this revision.
MyDeveloperDay added a comment.

I'm happy if you are, I think given that you've really been one of the main 
contributors of clang-format [JS] for the last number of years you are the best 
person to probably assess the impact on Javascript code.

Thanks for the patch, it LGTM and good luck!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Probst via Phabricator via cfe-commits]

mprobst marked 4 inline comments as done.
mprobst added inline comments.



Comment at: clang/include/clang/Format/Format.h:42
+  Unsuitable,
+  BinBackTrailingCommaConflict
+};

sammccall wrote:
> Back->Pack?
That's what you get when you fix my auto-complete: consistent misspelling.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

(Maybe we should have a basic test for C++, but I'm not sure how this usually 
goes)

BTW it occurs to me that turning this on by default may be a surprising 
breakage for:

- people who use clang-format to format JSON, where the comma is illegal.
- people who care about IE8 or something




Comment at: clang/include/clang/Format/Format.h:42
+  Unsuitable,
+  BinBackTrailingCommaConflict
+};

Back->Pack?



Comment at: clang/lib/Format/Format.cpp:2531
+});
+
   auto Env =

mprobst wrote:
> MyDeveloperDay wrote:
> > Ok, this comment is more a discussion point rather than a review comment. 
> > Was there a reason you needed to put the TrailingCommaInserter pass after 
> > the Formatter pass? (maybe you needed the Tokens annotated?)
> > 
> > From my understanding of some of the other conversations, it seemed the 
> > reason for ignoring the Column limit was because the "," insertion came 
> > after it had been formatted (is that correct?)
> > 
> > If there was a good reason that's also fine, I was just interested to learn 
> > if there was some part of the Formatter that perhaps needs to be pulled out 
> > into its own separate pass.
> > 
> > 
> The problem with inserting the comma during formatting is that it'd need to 
> be inserted during the state exploration as part of Dijkstra's implemented in 
> clang-format. I've considered that, but it'd be complex (if we make 
> formatting decision X, we now add a token, which might invalidate the 
> formatting decision). Keeping it as a separate pass has drawbacks, such as 
> potentially not ending up with perfect formatting, thus the backing off to 
> insert over ColumnLimit, but seems overall simpler.
> Ok, this comment is more a discussion point rather than a review comment. Was 
> there a reason you needed to put the TrailingCommaInserter pass after the 
> Formatter pass? (maybe you needed the Tokens annotated?)

The comma must only be inserted when the items are wrapped one-per-line (i.e. 
the last item is not on the same line as the closing bracket). This wrapping is 
a decision taken by the formatter, so we need to run it first to know.
(Apologies if this is obvious, but maybe missed)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Probst via Phabricator via cfe-commits]

mprobst marked 2 inline comments as done.
mprobst added inline comments.



Comment at: clang/lib/Format/Format.cpp:2531
+});
+
   auto Env =

MyDeveloperDay wrote:
> Ok, this comment is more a discussion point rather than a review comment. Was 
> there a reason you needed to put the TrailingCommaInserter pass after the 
> Formatter pass? (maybe you needed the Tokens annotated?)
> 
> From my understanding of some of the other conversations, it seemed the 
> reason for ignoring the Column limit was because the "," insertion came after 
> it had been formatted (is that correct?)
> 
> If there was a good reason that's also fine, I was just interested to learn 
> if there was some part of the Formatter that perhaps needs to be pulled out 
> into its own separate pass.
> 
> 
The problem with inserting the comma during formatting is that it'd need to be 
inserted during the state exploration as part of Dijkstra's implemented in 
clang-format. I've considered that, but it'd be complex (if we make formatting 
decision X, we now add a token, which might invalidate the formatting 
decision). Keeping it as a separate pass has drawbacks, such as potentially not 
ending up with perfect formatting, thus the backing off to insert over 
ColumnLimit, but seems overall simpler.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/Format.cpp:2531
+});
+
   auto Env =

Ok, this comment is more a discussion point rather than a review comment. Was 
there a reason you needed to put the TrailingCommaInserter pass after the 
Formatter pass? (maybe you needed the Tokens annotated?)

From my understanding of some of the other conversations, it seemed the reason 
for ignoring the Column limit was because the "," insertion came after it had 
been formatted (is that correct?)

If there was a good reason that's also fine, I was just interested to learn if 
there was some part of the Formatter that perhaps needs to be pulled out into 
its own separate pass.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Probst via Phabricator via cfe-commits]

mprobst marked 6 inline comments as done.
mprobst added a comment.

PTAL.




Comment at: clang/include/clang/Format/Format.h:552
+/// Insert trailing commas in container literals that were wrapped over
+/// multiple lines.
+TCS_Wrapped,

mprobst wrote:
> sammccall wrote:
> > Hadn't occurred to me that this will interact with bin-packing. That seems 
> > to increase the chances of going over the column limit :(
> Uh. Turns out we don't have a separate option for bin packing containers, 
> only for bin packing arguments and parameters.
> 
> Should we add that option to make this setting work?
> Should we have this setting imply bin packing containers (as opposed to 
> parameters) is disabled?
OK, so I added a bit of validation logic to reject bin packing arguments 
together with TSC_Wrapped. We can figure out in a next step whether we want to 
expose an option for bin packing containers, or whether we can just disable bin 
packing for those people who want trailing commas.



Comment at: clang/unittests/Format/FormatTestJS.cpp:1229
"  (param): param is {\n"
-   "a: SomeType\n"
+   "a: SomeType;\n"
"  } => 1)\n"

MyDeveloperDay wrote:
> mprobst wrote:
> > MyDeveloperDay wrote:
> > > is this correct?
> > Yes, type definitions should use `;`
> if you hadn't added this to the test would it have added a "," ?
More precisely:
you can  use either `;` or `,` to separate entries in object literal types. So 
using `;` is correct here, but c-f with this option would insert a `,`, which 
is correct as well. I've added a test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Probst via Phabricator via cfe-commits]

mprobst updated this revision to Diff 240873.
mprobst marked an inline comment as done.
mprobst added a comment.

- - only run comma insertion for JavaScript.
- review fixes
- Fix col limit
- test for comma insertion
- - validate options, reject bin packing + trailing commas


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354

Files:
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestJS.cpp

Index: clang/unittests/Format/FormatTestJS.cpp
===
--- clang/unittests/Format/FormatTestJS.cpp
+++ clang/unittests/Format/FormatTestJS.cpp
@@ -878,6 +878,45 @@
"]);");
 }
 
+TEST_F(FormatTestJS, TrailingCommaInsertion) {
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_JavaScript);
+  Style.InsertTrailingCommas = FormatStyle::TCS_Wrapped;
+  // Insert comma in wrapped array.
+  verifyFormat("const x = [\n"
+   "  1,  //\n"
+   "  2,\n"
+   "];",
+   "const x = [\n"
+   "  1,  //\n"
+   "  2];",
+   Style);
+  // Insert comma in newly wrapped array.
+  Style.ColumnLimit = 30;
+  verifyFormat("const x = [\n"
+   "  a,\n"
+   "];",
+   "const x = [a];", Style);
+  // Do not insert trailing commas if they'd exceed the colum limit
+  verifyFormat("const x = [\n"
+   "  \n"
+   "];",
+   "const x = [];", Style);
+  // Object literals.
+  verifyFormat("const x = {\n"
+   "  a: a,\n"
+   "};",
+   "const x = {a: a};", Style);
+  verifyFormat("const x = {\n"
+   "  a: a\n"
+   "};",
+   "const x = {a: a};", Style);
+  // Object literal types.
+  verifyFormat("let x: {\n"
+   "  a: a,\n"
+   "};",
+   "let x: {a: a};", Style);
+}
+
 TEST_F(FormatTestJS, FunctionLiterals) {
   FormatStyle Style = getGoogleStyle(FormatStyle::LK_JavaScript);
   Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13031,6 +13031,12 @@
"IndentWidth: 34",
),
 ParseError::Unsuitable);
+  FormatStyle BinPackedTCS = {};
+  BinPackedTCS.Language = FormatStyle::LK_JavaScript;
+  EXPECT_EQ(parseConfiguration("BinPackArguments: true\n"
+   "InsertTrailingCommas: Wrapped",
+   ),
+ParseError::BinBackTrailingCommaConflict);
   EXPECT_EQ(12u, Style.IndentWidth);
   CHECK_PARSE("IndentWidth: 56", IndentWidth, 56u);
   EXPECT_EQ(FormatStyle::LK_Cpp, Style.Language);
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -157,6 +157,13 @@
   }
 };
 
+template <> struct ScalarEnumerationTraits {
+  static void enumeration(IO , FormatStyle::TrailingCommaStyle ) {
+IO.enumCase(Value, "None", FormatStyle::TCS_None);
+IO.enumCase(Value, "Wrapped", FormatStyle::TCS_Wrapped);
+  }
+};
+
 template <> struct ScalarEnumerationTraits {
   static void enumeration(IO , FormatStyle::BinaryOperatorStyle ) {
 IO.enumCase(Value, "All", FormatStyle::BOS_All);
@@ -486,6 +493,7 @@
 IO.mapOptional("IndentWidth", Style.IndentWidth);
 IO.mapOptional("IndentWrappedFunctionNames",
Style.IndentWrappedFunctionNames);
+IO.mapOptional("InsertTrailingCommas", Style.InsertTrailingCommas);
 IO.mapOptional("JavaImportGroups", Style.JavaImportGroups);
 IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes);
 IO.mapOptional("JavaScriptWrapImports", Style.JavaScriptWrapImports);
@@ -644,6 +652,8 @@
 return "Invalid argument";
   case ParseError::Unsuitable:
 return "Unsuitable";
+  case ParseError::BinBackTrailingCommaConflict:
+return "trailing comma insertion cannot be used with bin packing";
   }
   llvm_unreachable("unexpected parse error");
 }
@@ -788,6 +798,7 @@
   LLVMStyle.IndentPPDirectives = FormatStyle::PPDIS_None;
   LLVMStyle.IndentWrappedFunctionNames = false;
   LLVMStyle.IndentWidth = 2;
+  LLVMStyle.InsertTrailingCommas = FormatStyle::TCS_None;
   LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave;
   LLVMStyle.JavaScriptWrapImports = true;
   LLVMStyle.TabWidth = 8;
@@ -946,6 +957,9 @@
 // taze:, triple 

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Nit: Could you add an entry for ReleaseNotes.rst and regenerate the 
ClangFormatStyleOption.rst with the docs/tools/dump_style.py




Comment at: clang/unittests/Format/FormatTestJS.cpp:1229
"  (param): param is {\n"
-   "a: SomeType\n"
+   "a: SomeType;\n"
"  } => 1)\n"

mprobst wrote:
> MyDeveloperDay wrote:
> > is this correct?
> Yes, type definitions should use `;`
if you hadn't added this to the test would it have added a "," ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Probst via Phabricator via cfe-commits]

mprobst marked 3 inline comments as done.
mprobst added inline comments.



Comment at: clang/include/clang/Format/Format.h:552
+/// Insert trailing commas in container literals that were wrapped over
+/// multiple lines.
+TCS_Wrapped,

sammccall wrote:
> Hadn't occurred to me that this will interact with bin-packing. That seems to 
> increase the chances of going over the column limit :(
Uh. Turns out we don't have a separate option for bin packing containers, only 
for bin packing arguments and parameters.

Should we add that option to make this setting work?
Should we have this setting imply bin packing containers (as opposed to 
parameters) is disabled?



Comment at: clang/lib/Format/Format.cpp:956
 GoogleStyle.CommentPragmas = "(taze:|^/[ \t]*<|tslint:|@see)";
+GoogleStyle.InsertTrailingCommas = FormatStyle::TCS_Wrapped;
 GoogleStyle.MaxEmptyLinesToKeep = 3;

MyDeveloperDay wrote:
> Are you sure the right decision is to make this on by default for something 
> that's going to insert the comma? is this in google's javascript style guide?
> 
> I think this could cause clang-format to suddenly start adding lost of commas 
> (as we see  with the tests)
It matches the style guide: 
https://google.github.io/styleguide/jsguide.html#features-arrays-trailing-comma

But it's indeed not entirely clear re bin packing, and certainly a good idea to 
split adding the option vs landing it.



Comment at: clang/lib/Format/Format.cpp:1477
 
+/// TrailingCommaInserter inserts trailing commas into container literals.
+/// E.g.:

MyDeveloperDay wrote:
> sammccall wrote:
> > Inlining this in format.cpp seems worse than having passes in their own 
> > files, but is the pattern so far. Ugh, up to you.
> Actually I think there is precedent to put TokenAnalyzers in their own class, 
> I don't think it should be in Format.cpp
Re inlining vs separate file, idk. On one hand, this file is on the large side, 
otoh these ~60 LOC don't carry their own weight in a separate file. Maybe in a 
follow up we should extract all passes into a separate file (maybe all passes 
except the actual indenter?).



Comment at: clang/lib/Format/Format.cpp:1482
+/// ];
+class TrailingCommaInserter : public TokenAnalyzer {
+public:

krasimir wrote:
> sammccall wrote:
> > 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.
> +1 for "Another policy that occurred to me: don't insert the comma if you're 
> exactly at the line limit." (or //over// the column limit). Seems like that 
> would be enough to keep it idempotent.
Done re comment, and also done re policy, good idea.

I've also added a comment that this doesn't work together with bin packing.



Comment at: clang/lib/Format/Format.cpp:1511
+FormatToken *Matching = FormatTok->MatchingParen;
+if (!Matching || !FormatTok->Previous)
+  continue;

sammccall wrote:
> 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)
Good catch. I think the right fix is to check if there is a previous non 
comment token, as that's what I'm using below.



Comment at: clang/unittests/Format/FormatTestJS.cpp:343
"  for: 'for',\n"
-   "  x: 'x'\n"
+   "  x: 'x',\n"
"};",

sammccall wrote:
> 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.
Ha, I just adjusted the test cases to my change, but forgot uploading the patch 
with the tests.

I've reverted all these changes (because I'm splitting flipping the default 
from this), but added an explicit test.



Comment at: clang/unittests/Format/FormatTestJS.cpp:1229
"  (param): param is {\n"
-   "a: SomeType\n"
+   "a: SomeType;\n"
"  } => 1)\n"

MyDeveloperDay wrote:
> is this correct?
Yes, type definitions should use `;`



Comment at: clang/unittests/Format/FormatTestJS.cpp:1701
   verifyFormat("type X = {\n"
-   "  y: number\n"
+   "  y: number;\n"
"};\n"

MyDeveloperDay wrote:
> is this correct?
Yes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  

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

[Martin Probst via Phabricator via cfe-commits]

mprobst updated this revision to Diff 240847.
mprobst marked 16 inline comments as done.
mprobst added a comment.

- - only run comma insertion for JavaScript.
- review fixes
- Fix col limit
- test for comma insertion


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354

Files:
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTestJS.cpp

Index: clang/unittests/Format/FormatTestJS.cpp
===
--- clang/unittests/Format/FormatTestJS.cpp
+++ clang/unittests/Format/FormatTestJS.cpp
@@ -878,6 +878,41 @@
"]);");
 }
 
+TEST_F(FormatTestJS, TrailingCommaInsertion) {
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_JavaScript);
+  Style.InsertTrailingCommas = FormatStyle::TCS_Wrapped;
+  // Insert comma in wrapped array.
+  verifyFormat("const x = [\n"
+   "  1,  //\n"
+   "  2,\n"
+   "];",
+   "const x = [\n"
+   "  1,  //\n"
+   "  2];",
+   Style);
+  // Insert comma in newly wrapped array.
+  Style.ColumnLimit = 30;
+  verifyFormat("const x = [\n"
+   "  a,\n"
+   "];",
+   "const x = [a];", Style);
+  // Do not insert trailing commas if they'd exceed the colum limit
+  verifyFormat("const x = [\n"
+   "  \n"
+   "];",
+   "const x = [];", Style);
+  // Object literals.
+  verifyFormat("const x = {\n"
+   "  a: a,\n"
+   "};",
+   "const x = {a: a};", Style);
+  verifyFormat("const x = {\n"
+   "  a: a\n"
+   "};",
+   "const x = {a: a};", Style);
+
+}
+
 TEST_F(FormatTestJS, FunctionLiterals) {
   FormatStyle Style = getGoogleStyle(FormatStyle::LK_JavaScript);
   Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -157,6 +157,13 @@
   }
 };
 
+template <> struct ScalarEnumerationTraits {
+  static void enumeration(IO , FormatStyle::TrailingCommaStyle ) {
+IO.enumCase(Value, "None", FormatStyle::TCS_None);
+IO.enumCase(Value, "Wrapped", FormatStyle::TCS_Wrapped);
+  }
+};
+
 template <> struct ScalarEnumerationTraits {
   static void enumeration(IO , FormatStyle::BinaryOperatorStyle ) {
 IO.enumCase(Value, "All", FormatStyle::BOS_All);
@@ -486,6 +493,7 @@
 IO.mapOptional("IndentWidth", Style.IndentWidth);
 IO.mapOptional("IndentWrappedFunctionNames",
Style.IndentWrappedFunctionNames);
+IO.mapOptional("InsertTrailingCommas", Style.InsertTrailingCommas);
 IO.mapOptional("JavaImportGroups", Style.JavaImportGroups);
 IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes);
 IO.mapOptional("JavaScriptWrapImports", Style.JavaScriptWrapImports);
@@ -788,6 +796,7 @@
   LLVMStyle.IndentPPDirectives = FormatStyle::PPDIS_None;
   LLVMStyle.IndentWrappedFunctionNames = false;
   LLVMStyle.IndentWidth = 2;
+  LLVMStyle.InsertTrailingCommas = FormatStyle::TCS_None;
   LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave;
   LLVMStyle.JavaScriptWrapImports = true;
   LLVMStyle.TabWidth = 8;
@@ -946,6 +955,9 @@
 // taze:, triple slash directives (`/// <...`), tslint:, and @see, which is
 // commonly followed by overlong URLs.
 GoogleStyle.CommentPragmas = "(taze:|^/[ \t]*<|tslint:|@see)";
+// TODO: enable once decided, in particular re disabling bin packing.
+// https://google.github.io/styleguide/jsguide.html#features-arrays-trailing-comma
+// GoogleStyle.InsertTrailingCommas = FormatStyle::TCS_Wrapped;
 GoogleStyle.MaxEmptyLinesToKeep = 3;
 GoogleStyle.NamespaceIndentation = FormatStyle::NI_All;
 GoogleStyle.SpacesInContainerLiterals = false;
@@ -1466,6 +1478,75 @@
   FormattingAttemptStatus *Status;
 };
 
+/// TrailingCommaInserter inserts trailing commas into container literals.
+/// E.g.:
+/// const x = [
+///   1,
+/// ];
+/// TrailingCommaInserter runs after formatting. To avoid causing a required
+/// reformatting (and thus reflow), it never inserts a comma that'd exceed the
+/// ColumnLimit.
+///
+/// Because trailing commas disable binpacking of arrays, TrailingCommaInserter
+/// is conceptually incompatible with bin packing.
+class TrailingCommaInserter : public TokenAnalyzer {
+public:
+  TrailingCommaInserter(const Environment , const FormatStyle )
+  : TokenAnalyzer(Env, Style) {}
+
+  std::pair
+  analyze(TokenAnnotator ,
+  SmallVectorImpl ,
+  FormatTokenLexer ) override {
+

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

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added inline comments.



Comment at: clang/lib/Format/Format.cpp:1482
+/// ];
+class TrailingCommaInserter : public TokenAnalyzer {
+public:

sammccall wrote:
> 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.
+1 for "Another policy that occurred to me: don't insert the comma if you're 
exactly at the line limit." (or //over// the column limit). Seems like that 
would be enough to keep it idempotent.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

FYI maybe related D33029: [clang-format] add option for dangling parenthesis 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added a comment.

An alternative approach (I'm not endorsing this) that would *in theory* 
circumvent non-idempotency issue (but would be more fiddly~fiddly to implement 
and spill across different layers of the formatter):
This is just a rough idea and can have multiple issues in itself. In general 
the trick is that if we can teach the token penalty computer and the format 
outputter about this, we should be able to use it together with other 
formatting decisions.

- add a new synthetic (annotated) token type, something like 
TT_PossiblyMissingTrailingComma, that would capture the length-0 text range 
where this could be inserted;
- use the algorithm here to splay it into the annotated token stream;
- mark it so that no newline can be inserted before such a token
- modify the penalty calculation algorithm to penalize for inserting a newline 
after this token: if we're inserting a newline after this token and if this 
token was inserted at a position at or past the maximal number of columns, add 
a PenaltyExcessCharacter for it.
- during outputting, if this token is followed by a newline, output it as `,`; 
otherwise output it as an empty string or do not output it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I'm a little uncomfortable about all the tests changing, and I'm unsure if we 
should be changing the default behaviour.

The rules in the past here was to show that this is part of a public style 
guide. The assumption here is google style wants this. Could you please point 
to that documentation so at least there is some comeback when we break the 
world.




Comment at: clang/lib/Format/Format.cpp:956
 GoogleStyle.CommentPragmas = "(taze:|^/[ \t]*<|tslint:|@see)";
+GoogleStyle.InsertTrailingCommas = FormatStyle::TCS_Wrapped;
 GoogleStyle.MaxEmptyLinesToKeep = 3;

Are you sure the right decision is to make this on by default for something 
that's going to insert the comma? is this in google's javascript style guide?

I think this could cause clang-format to suddenly start adding lost of commas 
(as we see  with the tests)



Comment at: clang/lib/Format/Format.cpp:1477
 
+/// TrailingCommaInserter inserts trailing commas into container literals.
+/// E.g.:

sammccall wrote:
> Inlining this in format.cpp seems worse than having passes in their own 
> files, but is the pattern so far. Ugh, up to you.
Actually I think there is precedent to put TokenAnalyzers in their own class, I 
don't think it should be in Format.cpp



Comment at: clang/unittests/Format/FormatTestJS.cpp:1229
"  (param): param is {\n"
-   "a: SomeType\n"
+   "a: SomeType;\n"
"  } => 1)\n"

is this correct?



Comment at: clang/unittests/Format/FormatTestJS.cpp:1701
   verifyFormat("type X = {\n"
-   "  y: number\n"
+   "  y: number;\n"
"};\n"

is this correct?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D73354#1838964 , @Bouska wrote:

> I have an issue with this change. Currently (at least for C++), the presence 
> of a trailing comma is used as a formatting hint to put all the element in 
> one line or one per line as below:


Good point about the interaction with bin-packing.
Do the style guides that want this actually endorse bin-packing containers? 
Insertion + bin-packing + comma-as-hint basically can't coexist - we could turn 
off the hint or make this option mutually exclusive with bin-packing.

> ...
>  So I definitely think this change should be done before formatting and not 
> after.

I don't think this is a solution - we might wrap a container and fail to insert 
the comma :-)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Pablo Martin-Gomez via Phabricator via cfe-commits]

Bouska added a comment.

I have an issue with this change. Currently (at least for C++), the presence of 
a trailing comma is used as a formatting hint to put all the element in one 
line or one per line as below:

  enum test1 = {ONE, TWO, THREE};
  
  enum test2 = {
  ONE,
  TWO,
  THREE,
  };

As your change come after the formatting, clang-format won't be idempotent 
anymore. You are going to have the following if the container is bin packed (I 
did not actually test this):

  First run:  
  enum test1 = {
  ONE, TWO,
  THREE,};
  
  Second run:
  enum test1 = {
  ONE,
  TWO,
  THREE,
  };

So I definitely think this change should be done before formatting and not 
after.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Pablo Martin-Gomez via Phabricator via cfe-commits]

Bouska added a comment.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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"
+   "  aaa,\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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added a comment.

General comment:

I think this is useful and could also be useful for other languages too (some 
folks have asked for this for some C++ code).
As this is a non-whitespace only change, it's a bit more risky than 
whitespace-only changes.

I think we need to establish a policy on such more riskier style options, at 
least have a convenient way for users to to opt-out of such and be extra 
careful including them by default in default styles.
This is tricky because some style guides explicitly recommend such changes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Probst via Phabricator via cfe-commits]

mprobst updated this revision to Diff 240194.
mprobst added a comment.

- - only run comma insertion for JavaScript.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354

Files:
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTestJS.cpp

Index: clang/unittests/Format/FormatTestJS.cpp
===
--- clang/unittests/Format/FormatTestJS.cpp
+++ clang/unittests/Format/FormatTestJS.cpp
@@ -340,7 +340,7 @@
"}\n");
   verifyFormat("const Axis = {\n"
"  for: 'for',\n"
-   "  x: 'x'\n"
+   "  x: 'x',\n"
"};",
"const Axis = {for: 'for', x:   'x'};");
 }
@@ -405,18 +405,18 @@
   verifyFormat("var x = {\n"
"  y: function(a) {\n"
"return a;\n"
-   "  }\n"
+   "  },\n"
"};");
   verifyFormat("return {\n"
"  link: function() {\n"
"f();  //\n"
-   "  }\n"
+   "  },\n"
"};");
   verifyFormat("return {\n"
"  a: a,\n"
"  link: function() {\n"
"f();  //\n"
-   "  }\n"
+   "  },\n"
"};");
   verifyFormat("return {\n"
"  a: a,\n"
@@ -425,7 +425,7 @@
"  },\n"
"  link: function() {\n"
"f();  //\n"
-   "  }\n"
+   "  },\n"
"};");
   verifyFormat("var stuff = {\n"
"  // comment for update\n"
@@ -433,23 +433,27 @@
"  // comment for modules\n"
"  modules: false,\n"
"  // comment for tasks\n"
-   "  tasks: false\n"
+   "  tasks: false,\n"
"};");
   verifyFormat("return {\n"
"  'finish':\n"
"  //\n"
-   "  a\n"
+   "  a,\n"
"};");
   verifyFormat("var obj = {\n"
"  fo: function(x) {\n"
"return x.zIsTooLongForOneLineWithTheDeclarationLine();\n"
-   "  }\n"
+   "  },\n"
"};");
   // Simple object literal, as opposed to enum style below.
   verifyFormat("var obj = {a: 123};");
   // Enum style top level assignment.
-  verifyFormat("X = {\n  a: 123\n};");
-  verifyFormat("X.Y = {\n  a: 123\n};");
+  verifyFormat("X = {\n"
+   "  a: 123,\n"
+   "};");
+  verifyFormat("X.Y = {\n"
+   "  a: 123,\n"
+   "};");
   // But only on the top level, otherwise its a plain object literal assignment.
   verifyFormat("function x() {\n"
"  y = {z: 1};\n"
@@ -460,7 +464,7 @@
   verifyFormat("var x = {\n"
"  y: (a) => {\n"
"return a;\n"
-   "  }\n"
+   "  },\n"
"};");
   verifyFormat("var x = {y: (a) => a};");
 
@@ -468,12 +472,12 @@
   verifyFormat("var x = {\n"
"  y(a: string): number {\n"
"return a;\n"
-   "  }\n"
+   "  },\n"
"};");
   verifyFormat("var x = {\n"
"  y(a: string) {\n"
"return a;\n"
-   "  }\n"
+   "  },\n"
"};");
 
   // Computed keys.
@@ -514,19 +518,19 @@
"  value: 'test',\n"
"  get value() {  // getter\n"
"return this.value;\n"
-   "  }\n"
+   "  },\n"
"};");
   verifyFormat("var o = {\n"
"  value: 'test',\n"
"  set value(val) {  // setter\n"
"this.value = val;\n"
-   "  }\n"
+   "  },\n"
"};");
   verifyFormat("var o = {\n"
"  value: 'test',\n"
"  someMethod(val) {  // method\n"
"doSomething(this.value + val);\n"
-   "  }\n"
+   "  },\n"
"};");
   verifyFormat("var o = {\n"
"  someMethod(val) {  // method\n"
@@ -534,7 +538,7 @@
"  },\n"
"  someOtherMethod(val) {  // method\n"
"doSomething(this.value + val);\n"
-   "  }\n"
+   "  },\n"
"};");
 }
 
@@ -563,7 +567,7 @@
 
   verifyFormat("var object_literal_with_long_name = {\n"
"  a: 'aa',\n"
-   "  b: 'bb'\n"
+   "  b: 'bb',\n"
"};");
 
   verifyFormat("f({a: 1, b: 2, c: 3});",
@@ -730,7 +734,7 @@
   verifyFormat("var x = {\n"
"  a: function*() {\n"
"//\n"
-   "  }\n"
+   " 

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

[Martin Probst via Phabricator via cfe-commits]

mprobst added a comment.

This is required by JavaScript style, but this change as is optimistically runs 
the tool for any language. I think that's probably not what we want here 
initially, just throwing it out there. WDYT?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Probst via Phabricator via cfe-commits]

mprobst created this revision.
mprobst added reviewers: krasimir, MyDeveloperDay.
Herald added a project: clang.

This change adds an option to insert trailing commas into container
literals. For example, in JavaScript:

  const x = [
a,
b,
 ^ inserted if missing.
  ]

This is implemented as a seperate post-processing pass after formatting
(because formatting might change whether the container literal does or
does not wrap). This keeps the code relatively simple and orthogonal,
though it has the notable drawback that the newly inserted comma is not
taken into account for formatting decisions (e.g. it might exceed the 80
char limit).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73354

Files:
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTestJS.cpp

Index: clang/unittests/Format/FormatTestJS.cpp
===
--- clang/unittests/Format/FormatTestJS.cpp
+++ clang/unittests/Format/FormatTestJS.cpp
@@ -340,7 +340,7 @@
"}\n");
   verifyFormat("const Axis = {\n"
"  for: 'for',\n"
-   "  x: 'x'\n"
+   "  x: 'x',\n"
"};",
"const Axis = {for: 'for', x:   'x'};");
 }
@@ -405,18 +405,18 @@
   verifyFormat("var x = {\n"
"  y: function(a) {\n"
"return a;\n"
-   "  }\n"
+   "  },\n"
"};");
   verifyFormat("return {\n"
"  link: function() {\n"
"f();  //\n"
-   "  }\n"
+   "  },\n"
"};");
   verifyFormat("return {\n"
"  a: a,\n"
"  link: function() {\n"
"f();  //\n"
-   "  }\n"
+   "  },\n"
"};");
   verifyFormat("return {\n"
"  a: a,\n"
@@ -425,7 +425,7 @@
"  },\n"
"  link: function() {\n"
"f();  //\n"
-   "  }\n"
+   "  },\n"
"};");
   verifyFormat("var stuff = {\n"
"  // comment for update\n"
@@ -433,23 +433,27 @@
"  // comment for modules\n"
"  modules: false,\n"
"  // comment for tasks\n"
-   "  tasks: false\n"
+   "  tasks: false,\n"
"};");
   verifyFormat("return {\n"
"  'finish':\n"
"  //\n"
-   "  a\n"
+   "  a,\n"
"};");
   verifyFormat("var obj = {\n"
"  fo: function(x) {\n"
"return x.zIsTooLongForOneLineWithTheDeclarationLine();\n"
-   "  }\n"
+   "  },\n"
"};");
   // Simple object literal, as opposed to enum style below.
   verifyFormat("var obj = {a: 123};");
   // Enum style top level assignment.
-  verifyFormat("X = {\n  a: 123\n};");
-  verifyFormat("X.Y = {\n  a: 123\n};");
+  verifyFormat("X = {\n"
+   "  a: 123,\n"
+   "};");
+  verifyFormat("X.Y = {\n"
+   "  a: 123,\n"
+   "};");
   // But only on the top level, otherwise its a plain object literal assignment.
   verifyFormat("function x() {\n"
"  y = {z: 1};\n"
@@ -460,7 +464,7 @@
   verifyFormat("var x = {\n"
"  y: (a) => {\n"
"return a;\n"
-   "  }\n"
+   "  },\n"
"};");
   verifyFormat("var x = {y: (a) => a};");
 
@@ -468,12 +472,12 @@
   verifyFormat("var x = {\n"
"  y(a: string): number {\n"
"return a;\n"
-   "  }\n"
+   "  },\n"
"};");
   verifyFormat("var x = {\n"
"  y(a: string) {\n"
"return a;\n"
-   "  }\n"
+   "  },\n"
"};");
 
   // Computed keys.
@@ -514,19 +518,19 @@
"  value: 'test',\n"
"  get value() {  // getter\n"
"return this.value;\n"
-   "  }\n"
+   "  },\n"
"};");
   verifyFormat("var o = {\n"
"  value: 'test',\n"
"  set value(val) {  // setter\n"
"this.value = val;\n"
-   "  }\n"
+   "  },\n"
"};");
   verifyFormat("var o = {\n"
"  value: 'test',\n"
"  someMethod(val) {  // method\n"
"doSomething(this.value + val);\n"
-   "  }\n"
+   "  },\n"
"};");
   verifyFormat("var o = {\n"
"  someMethod(val) {  // method\n"
@@ -534,7 +538,7 @@
"  },\n"
"  someOtherMethod(val) {  // method\n"
"doSomething(this.value + val);\n"
-   "  }\n"
+   "