[PATCH] D36142: clang-format: [JS] do not insert whitespace in call positions.

[Martin Probst via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL310851: clang-format: [JS] do not insert whitespace in call 
positions. (authored by mprobst).

Repository:
  rL LLVM

https://reviews.llvm.org/D36142

Files:
  cfe/trunk/lib/Format/TokenAnnotator.cpp
  cfe/trunk/unittests/Format/FormatTestJS.cpp


Index: cfe/trunk/lib/Format/TokenAnnotator.cpp
===
--- cfe/trunk/lib/Format/TokenAnnotator.cpp
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp
@@ -2367,13 +2367,20 @@
 Left.isOneOf(Keywords.kw_function, Keywords.kw_yield,
  Keywords.kw_extends, Keywords.kw_implements))
   return true;
-// JS methods can use some keywords as names (e.g. `delete()`).
-if (Right.is(tok::l_paren) && Line.MustBeDeclaration &&
-Left.Tok.getIdentifierInfo())
-  return false;
-if (Right.is(tok::l_paren) &&
-Left.isOneOf(tok::kw_throw, Keywords.kw_await, Keywords.kw_typeof, 
tok::kw_void))
-  return true;
+if (Right.is(tok::l_paren)) {
+  // JS methods can use some keywords as names (e.g. `delete()`).
+  if (Line.MustBeDeclaration && Left.Tok.getIdentifierInfo())
+return false;
+  // Valid JS method names can include keywords, e.g. `foo.delete()` or
+  // `bar.instanceof()`. Recognize call positions by preceding period.
+  if (Left.Previous && Left.Previous->is(tok::period) &&
+  Left.Tok.getIdentifierInfo())
+return false;
+  // Additional unary JavaScript operators that need a space after.
+  if (Left.isOneOf(tok::kw_throw, Keywords.kw_await, Keywords.kw_typeof,
+   tok::kw_void))
+return true;
+}
 if ((Left.isOneOf(Keywords.kw_let, Keywords.kw_var, Keywords.kw_in,
   tok::kw_const) ||
  // "of" is only a keyword if it appears after another identifier
Index: cfe/trunk/unittests/Format/FormatTestJS.cpp
===
--- cfe/trunk/unittests/Format/FormatTestJS.cpp
+++ cfe/trunk/unittests/Format/FormatTestJS.cpp
@@ -271,14 +271,21 @@
   verifyFormat("x.case = 1;");
   verifyFormat("x.interface = 1;");
   verifyFormat("x.for = 1;");
-  verifyFormat("x.of() = 1;");
+  verifyFormat("x.of();");
   verifyFormat("of(null);");
   verifyFormat("import {of} from 'x';");
-  verifyFormat("x.in() = 1;");
-  verifyFormat("x.let() = 1;");
-  verifyFormat("x.var() = 1;");
-  verifyFormat("x.for() = 1;");
-  verifyFormat("x.as() = 1;");
+  verifyFormat("x.in();");
+  verifyFormat("x.let();");
+  verifyFormat("x.var();");
+  verifyFormat("x.for();");
+  verifyFormat("x.as();");
+  verifyFormat("x.instanceof();");
+  verifyFormat("x.switch();");
+  verifyFormat("x.case();");
+  verifyFormat("x.delete();");
+  verifyFormat("x.throw();");
+  verifyFormat("x.throws();");
+  verifyFormat("x.if();");
   verifyFormat("x = {\n"
"  a: 12,\n"
"  interface: 1,\n"
@@ -1228,7 +1235,6 @@
   // But, of course, "catch" is a perfectly fine function name in JavaScript.
   verifyFormat("someObject.catch();");
   verifyFormat("someObject.new();");
-  verifyFormat("someObject.delete();");
 }
 
 TEST_F(FormatTestJS, StringLiteralConcatenation) {


Index: cfe/trunk/lib/Format/TokenAnnotator.cpp
===
--- cfe/trunk/lib/Format/TokenAnnotator.cpp
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp
@@ -2367,13 +2367,20 @@
 Left.isOneOf(Keywords.kw_function, Keywords.kw_yield,
  Keywords.kw_extends, Keywords.kw_implements))
   return true;
-// JS methods can use some keywords as names (e.g. `delete()`).
-if (Right.is(tok::l_paren) && Line.MustBeDeclaration &&
-Left.Tok.getIdentifierInfo())
-  return false;
-if (Right.is(tok::l_paren) &&
-Left.isOneOf(tok::kw_throw, Keywords.kw_await, Keywords.kw_typeof, tok::kw_void))
-  return true;
+if (Right.is(tok::l_paren)) {
+  // JS methods can use some keywords as names (e.g. `delete()`).
+  if (Line.MustBeDeclaration && Left.Tok.getIdentifierInfo())
+return false;
+  // Valid JS method names can include keywords, e.g. `foo.delete()` or
+  // `bar.instanceof()`. Recognize call positions by preceding period.
+  if (Left.Previous && Left.Previous->is(tok::period) &&
+  Left.Tok.getIdentifierInfo())
+return false;
+  // Additional unary JavaScript operators that need a space after.
+  if (Left.isOneOf(tok::kw_throw, Keywords.kw_await, Keywords.kw_typeof,
+   tok::kw_void))
+return true;
+}
 if ((Left.isOneOf(Keywords.kw_let, Keywords.kw_var, Keywords.kw_in,
   tok::kw_const) ||
  // "of" is only a keyword if it appears after another identifier
Index: cfe/trunk/unittests/Format/FormatTestJS.cpp
=

[PATCH] D36142: clang-format: [JS] do not insert whitespace in call positions.

[Daniel Jasper via Phabricator via cfe-commits]

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

Looks good.


https://reviews.llvm.org/D36142



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

[PATCH] D36142: clang-format: [JS] do not insert whitespace in call positions.

[Martin Probst via Phabricator via cfe-commits]

mprobst added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:2355
+(Left.Tok.getIdentifierInfo() ||
+ Left.isOneOf(tok::kw_switch, tok::kw_case, tok::kw_delete)))
+  return false;

djasper wrote:
> Why is instanceof not required in this list?
`instanceof` is not a keyword in the C++ sense, so it is covered by the 
`getIdentifierInfo` part. But this was actually a misunderstanding - 
`getIdentifierInfo` actually covers all keyword-ish tokens, so we don't need 
the list of tokens here at all.


https://reviews.llvm.org/D36142



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

[PATCH] D36142: clang-format: [JS] do not insert whitespace in call positions.

[Martin Probst via Phabricator via cfe-commits]

mprobst updated this revision to Diff 110959.
mprobst marked 2 inline comments as done.
mprobst added a comment.

- clean up implementation, just use getIdentifierInfo
- clean up tests, remove illegal syntax, add more tests


https://reviews.llvm.org/D36142

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTestJS.cpp


Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -271,14 +271,21 @@
   verifyFormat("x.case = 1;");
   verifyFormat("x.interface = 1;");
   verifyFormat("x.for = 1;");
-  verifyFormat("x.of() = 1;");
+  verifyFormat("x.of();");
   verifyFormat("of(null);");
   verifyFormat("import {of} from 'x';");
-  verifyFormat("x.in() = 1;");
-  verifyFormat("x.let() = 1;");
-  verifyFormat("x.var() = 1;");
-  verifyFormat("x.for() = 1;");
-  verifyFormat("x.as() = 1;");
+  verifyFormat("x.in();");
+  verifyFormat("x.let();");
+  verifyFormat("x.var();");
+  verifyFormat("x.for();");
+  verifyFormat("x.as();");
+  verifyFormat("x.instanceof();");
+  verifyFormat("x.switch();");
+  verifyFormat("x.case();");
+  verifyFormat("x.delete();");
+  verifyFormat("x.throw();");
+  verifyFormat("x.throws();");
+  verifyFormat("x.if();");
   verifyFormat("x = {\n"
"  a: 12,\n"
"  interface: 1,\n"
@@ -1228,7 +1235,6 @@
   // But, of course, "catch" is a perfectly fine function name in JavaScript.
   verifyFormat("someObject.catch();");
   verifyFormat("someObject.new();");
-  verifyFormat("someObject.delete();");
 }
 
 TEST_F(FormatTestJS, StringLiteralConcatenation) {
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2367,13 +2367,20 @@
 Left.isOneOf(Keywords.kw_function, Keywords.kw_yield,
  Keywords.kw_extends, Keywords.kw_implements))
   return true;
-// JS methods can use some keywords as names (e.g. `delete()`).
-if (Right.is(tok::l_paren) && Line.MustBeDeclaration &&
-Left.Tok.getIdentifierInfo())
-  return false;
-if (Right.is(tok::l_paren) &&
-Left.isOneOf(tok::kw_throw, Keywords.kw_await, Keywords.kw_typeof, 
tok::kw_void))
-  return true;
+if (Right.is(tok::l_paren)) {
+  // JS methods can use some keywords as names (e.g. `delete()`).
+  if (Line.MustBeDeclaration && Left.Tok.getIdentifierInfo())
+return false;
+  // Valid JS method names can include keywords, e.g. `foo.delete()` or
+  // `bar.instanceof()`. Recognize call positions by preceding period.
+  if (Left.Previous && Left.Previous->is(tok::period) &&
+  Left.Tok.getIdentifierInfo())
+return false;
+  // Additional unary JavaScript operators that need a space after.
+  if (Left.isOneOf(tok::kw_throw, Keywords.kw_await, Keywords.kw_typeof,
+   tok::kw_void))
+return true;
+}
 if ((Left.isOneOf(Keywords.kw_let, Keywords.kw_var, Keywords.kw_in,
   tok::kw_const) ||
  // "of" is only a keyword if it appears after another identifier


Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -271,14 +271,21 @@
   verifyFormat("x.case = 1;");
   verifyFormat("x.interface = 1;");
   verifyFormat("x.for = 1;");
-  verifyFormat("x.of() = 1;");
+  verifyFormat("x.of();");
   verifyFormat("of(null);");
   verifyFormat("import {of} from 'x';");
-  verifyFormat("x.in() = 1;");
-  verifyFormat("x.let() = 1;");
-  verifyFormat("x.var() = 1;");
-  verifyFormat("x.for() = 1;");
-  verifyFormat("x.as() = 1;");
+  verifyFormat("x.in();");
+  verifyFormat("x.let();");
+  verifyFormat("x.var();");
+  verifyFormat("x.for();");
+  verifyFormat("x.as();");
+  verifyFormat("x.instanceof();");
+  verifyFormat("x.switch();");
+  verifyFormat("x.case();");
+  verifyFormat("x.delete();");
+  verifyFormat("x.throw();");
+  verifyFormat("x.throws();");
+  verifyFormat("x.if();");
   verifyFormat("x = {\n"
"  a: 12,\n"
"  interface: 1,\n"
@@ -1228,7 +1235,6 @@
   // But, of course, "catch" is a perfectly fine function name in JavaScript.
   verifyFormat("someObject.catch();");
   verifyFormat("someObject.new();");
-  verifyFormat("someObject.delete();");
 }
 
 TEST_F(FormatTestJS, StringLiteralConcatenation) {
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2367,13 +2367,20 @@
 Left.isOneOf(Keywords.kw_function, Keywords.kw_yield,
  Keywords.kw_extends, Keywords.kw_implements))
   return true;
-// JS methods can use some keywords as names (e.g. `delete()`).
-if 

[PATCH] D36142: clang-format: [JS] do not insert whitespace in call positions.

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:2355
+(Left.Tok.getIdentifierInfo() ||
+ Left.isOneOf(tok::kw_switch, tok::kw_case, tok::kw_delete)))
+  return false;

Why is instanceof not required in this list?



Comment at: unittests/Format/FormatTestJS.cpp:237
+  verifyFormat("x.switch() = 1;");
+  verifyFormat("x.case() = 1;");
   verifyFormat("x = {\n"

no test for delete?


https://reviews.llvm.org/D36142



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

[PATCH] D36142: clang-format: [JS] do not insert whitespace in call positions.

[Martin Probst via Phabricator via cfe-commits]

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

- support switch, case, delete.


https://reviews.llvm.org/D36142

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTestJS.cpp


Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -232,6 +232,9 @@
   verifyFormat("x.var() = 1;");
   verifyFormat("x.for() = 1;");
   verifyFormat("x.as() = 1;");
+  verifyFormat("x.instanceof() = 1;");
+  verifyFormat("x.switch() = 1;");
+  verifyFormat("x.case() = 1;");
   verifyFormat("x = {\n"
"  a: 12,\n"
"  interface: 1,\n"
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2347,6 +2347,13 @@
 if (Right.is(tok::l_paren) && Line.MustBeDeclaration &&
 Left.Tok.getIdentifierInfo())
   return false;
+// Valid JS method names can include keywords, e.g. `foo.delete()` or
+// `bar.instanceof()`.
+if (Right.is(tok::l_paren) && Left.Previous &&
+Left.Previous->is(tok::period) &&
+(Left.Tok.getIdentifierInfo() ||
+ Left.isOneOf(tok::kw_switch, tok::kw_case, tok::kw_delete)))
+  return false;
 if ((Left.isOneOf(Keywords.kw_let, Keywords.kw_var, Keywords.kw_in,
   tok::kw_const) ||
  // "of" is only a keyword if it appears after another identifier


Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -232,6 +232,9 @@
   verifyFormat("x.var() = 1;");
   verifyFormat("x.for() = 1;");
   verifyFormat("x.as() = 1;");
+  verifyFormat("x.instanceof() = 1;");
+  verifyFormat("x.switch() = 1;");
+  verifyFormat("x.case() = 1;");
   verifyFormat("x = {\n"
"  a: 12,\n"
"  interface: 1,\n"
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2347,6 +2347,13 @@
 if (Right.is(tok::l_paren) && Line.MustBeDeclaration &&
 Left.Tok.getIdentifierInfo())
   return false;
+// Valid JS method names can include keywords, e.g. `foo.delete()` or
+// `bar.instanceof()`.
+if (Right.is(tok::l_paren) && Left.Previous &&
+Left.Previous->is(tok::period) &&
+(Left.Tok.getIdentifierInfo() ||
+ Left.isOneOf(tok::kw_switch, tok::kw_case, tok::kw_delete)))
+  return false;
 if ((Left.isOneOf(Keywords.kw_let, Keywords.kw_var, Keywords.kw_in,
   tok::kw_const) ||
  // "of" is only a keyword if it appears after another identifier
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36142: clang-format: [JS] do not insert whitespace in call positions.

[Martin Probst via Phabricator via cfe-commits]

mprobst created this revision.
Herald added a subscriber: klimek.

In JavaScript, may keywords can be used in method names and thus call sites:

  foo.delete();
  foo.instanceof();

clang-format would previously insert whitespace after the `instanceof`. This
change generically skips inserting whitespace between a keyword and a
parenthesis if preceded by a dot, i.e. in a callsite.


https://reviews.llvm.org/D36142

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTestJS.cpp


Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -232,6 +232,7 @@
   verifyFormat("x.var() = 1;");
   verifyFormat("x.for() = 1;");
   verifyFormat("x.as() = 1;");
+  verifyFormat("x.instanceof() = 1;");
   verifyFormat("x = {\n"
"  a: 12,\n"
"  interface: 1,\n"
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2347,6 +2347,11 @@
 if (Right.is(tok::l_paren) && Line.MustBeDeclaration &&
 Left.Tok.getIdentifierInfo())
   return false;
+// Valid JS method names can include keywords, e.g. `foo.delete()` or
+// `bar.instanceof()`.
+if (Right.is(tok::l_paren) && Left.Tok.getIdentifierInfo() &&
+Left.Previous && Left.Previous->is(tok::period))
+  return false;
 if ((Left.isOneOf(Keywords.kw_let, Keywords.kw_var, Keywords.kw_in,
   tok::kw_const) ||
  // "of" is only a keyword if it appears after another identifier


Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -232,6 +232,7 @@
   verifyFormat("x.var() = 1;");
   verifyFormat("x.for() = 1;");
   verifyFormat("x.as() = 1;");
+  verifyFormat("x.instanceof() = 1;");
   verifyFormat("x = {\n"
"  a: 12,\n"
"  interface: 1,\n"
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2347,6 +2347,11 @@
 if (Right.is(tok::l_paren) && Line.MustBeDeclaration &&
 Left.Tok.getIdentifierInfo())
   return false;
+// Valid JS method names can include keywords, e.g. `foo.delete()` or
+// `bar.instanceof()`.
+if (Right.is(tok::l_paren) && Left.Tok.getIdentifierInfo() &&
+Left.Previous && Left.Previous->is(tok::period))
+  return false;
 if ((Left.isOneOf(Keywords.kw_let, Keywords.kw_var, Keywords.kw_in,
   tok::kw_const) ||
  // "of" is only a keyword if it appears after another identifier
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits