Re: [PATCH] D21658: clang-format: [JS] handle conditionals in fields, default params.

[Martin Probst via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL273619: clang-format: [JS] handle conditionals in fields, 
default params. (authored by mprobst).

Changed prior to commit:
  http://reviews.llvm.org/D21658?vs=61726=61729#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D21658

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
@@ -639,7 +639,7 @@
   }
   // Declarations cannot be conditional expressions, this can only be part
   // of a type declaration.
-  if (Line.MustBeDeclaration &&
+  if (Line.MustBeDeclaration && !Contexts.back().IsExpression &&
   Style.Language == FormatStyle::LK_JavaScript)
 break;
   parseConditional();
@@ -1009,7 +1009,7 @@
   Current.Type = TT_UnaryOperator;
 } else if (Current.is(tok::question)) {
   if (Style.Language == FormatStyle::LK_JavaScript &&
-  Line.MustBeDeclaration) {
+  Line.MustBeDeclaration && !Contexts.back().IsExpression) {
 // In JavaScript, `interface X { foo?(): bar; }` is an optional method
 // on the interface, not a ternary expression.
 Current.Type = TT_JsTypeOptionalQuestion;
Index: cfe/trunk/unittests/Format/FormatTestJS.cpp
===
--- cfe/trunk/unittests/Format/FormatTestJS.cpp
+++ cfe/trunk/unittests/Format/FormatTestJS.cpp
@@ -1253,7 +1253,6 @@
   verifyFormat("interface X {\n"
"  y?(): z;\n"
"}");
-  verifyFormat("x ? 1 : 2;");
   verifyFormat("constructor({aa}: {\n"
"  aa?: string,\n"
"  ?: string,\n"
@@ -1350,5 +1349,14 @@
   verifyFormat("let x = {foo: 1}!;\n");
 }
 
+TEST_F(FormatTestJS, Conditional) {
+  verifyFormat("y = x ? 1 : 2;");
+  verifyFormat("x ? 1 : 2;");
+  verifyFormat("class Foo {\n"
+   "  field = true ? 1 : 2;\n"
+   "  method(a = true ? 1 : 2) {}\n"
+   "}");
+}
+
 } // end namespace tooling
 } // end namespace clang


Index: cfe/trunk/lib/Format/TokenAnnotator.cpp
===
--- cfe/trunk/lib/Format/TokenAnnotator.cpp
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp
@@ -639,7 +639,7 @@
   }
   // Declarations cannot be conditional expressions, this can only be part
   // of a type declaration.
-  if (Line.MustBeDeclaration &&
+  if (Line.MustBeDeclaration && !Contexts.back().IsExpression &&
   Style.Language == FormatStyle::LK_JavaScript)
 break;
   parseConditional();
@@ -1009,7 +1009,7 @@
   Current.Type = TT_UnaryOperator;
 } else if (Current.is(tok::question)) {
   if (Style.Language == FormatStyle::LK_JavaScript &&
-  Line.MustBeDeclaration) {
+  Line.MustBeDeclaration && !Contexts.back().IsExpression) {
 // In JavaScript, `interface X { foo?(): bar; }` is an optional method
 // on the interface, not a ternary expression.
 Current.Type = TT_JsTypeOptionalQuestion;
Index: cfe/trunk/unittests/Format/FormatTestJS.cpp
===
--- cfe/trunk/unittests/Format/FormatTestJS.cpp
+++ cfe/trunk/unittests/Format/FormatTestJS.cpp
@@ -1253,7 +1253,6 @@
   verifyFormat("interface X {\n"
"  y?(): z;\n"
"}");
-  verifyFormat("x ? 1 : 2;");
   verifyFormat("constructor({aa}: {\n"
"  aa?: string,\n"
"  ?: string,\n"
@@ -1350,5 +1349,14 @@
   verifyFormat("let x = {foo: 1}!;\n");
 }
 
+TEST_F(FormatTestJS, Conditional) {
+  verifyFormat("y = x ? 1 : 2;");
+  verifyFormat("x ? 1 : 2;");
+  verifyFormat("class Foo {\n"
+   "  field = true ? 1 : 2;\n"
+   "  method(a = true ? 1 : 2) {}\n"
+   "}");
+}
+
 } // end namespace tooling
 } // end namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D21658: clang-format: [JS] handle conditionals in fields, default params.

[Daniel Jasper via cfe-commits]

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

Looks good except that the patch description can now be updated.


http://reviews.llvm.org/D21658



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

Re: [PATCH] D21658: clang-format: [JS] handle conditionals in fields, default params.

[Martin Probst via cfe-commits]

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

ok


http://reviews.llvm.org/D21658

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

Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -1247,7 +1247,6 @@
   verifyFormat("interface X {\n"
"  y?(): z;\n"
"}");
-  verifyFormat("x ? 1 : 2;");
   verifyFormat("constructor({aa}: {\n"
"  aa?: string,\n"
"  ?: string,\n"
@@ -1344,5 +1343,14 @@
   verifyFormat("let x = {foo: 1}!;\n");
 }
 
+TEST_F(FormatTestJS, Conditional) {
+  verifyFormat("y = x ? 1 : 2;");
+  verifyFormat("x ? 1 : 2;");
+  verifyFormat("class Foo {\n"
+   "  field = true ? 1 : 2;\n"
+   "  method(a = true ? 1 : 2) {}\n"
+   "}");
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -631,7 +631,7 @@
   }
   // Declarations cannot be conditional expressions, this can only be part
   // of a type declaration.
-  if (Line.MustBeDeclaration &&
+  if (Line.MustBeDeclaration && !Contexts.back().IsExpression &&
   Style.Language == FormatStyle::LK_JavaScript)
 break;
   parseConditional();
@@ -998,7 +998,7 @@
   Current.Type = TT_UnaryOperator;
 } else if (Current.is(tok::question)) {
   if (Style.Language == FormatStyle::LK_JavaScript &&
-  Line.MustBeDeclaration) {
+  Line.MustBeDeclaration && !Contexts.back().IsExpression) {
 // In JavaScript, `interface X { foo?(): bar; }` is an optional method
 // on the interface, not a ternary expression.
 Current.Type = TT_JsTypeOptionalQuestion;


Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -1247,7 +1247,6 @@
   verifyFormat("interface X {\n"
"  y?(): z;\n"
"}");
-  verifyFormat("x ? 1 : 2;");
   verifyFormat("constructor({aa}: {\n"
"  aa?: string,\n"
"  ?: string,\n"
@@ -1344,5 +1343,14 @@
   verifyFormat("let x = {foo: 1}!;\n");
 }
 
+TEST_F(FormatTestJS, Conditional) {
+  verifyFormat("y = x ? 1 : 2;");
+  verifyFormat("x ? 1 : 2;");
+  verifyFormat("class Foo {\n"
+   "  field = true ? 1 : 2;\n"
+   "  method(a = true ? 1 : 2) {}\n"
+   "}");
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -631,7 +631,7 @@
   }
   // Declarations cannot be conditional expressions, this can only be part
   // of a type declaration.
-  if (Line.MustBeDeclaration &&
+  if (Line.MustBeDeclaration && !Contexts.back().IsExpression &&
   Style.Language == FormatStyle::LK_JavaScript)
 break;
   parseConditional();
@@ -998,7 +998,7 @@
   Current.Type = TT_UnaryOperator;
 } else if (Current.is(tok::question)) {
   if (Style.Language == FormatStyle::LK_JavaScript &&
-  Line.MustBeDeclaration) {
+  Line.MustBeDeclaration && !Contexts.back().IsExpression) {
 // In JavaScript, `interface X { foo?(): bar; }` is an optional method
 // on the interface, not a ternary expression.
 Current.Type = TT_JsTypeOptionalQuestion;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D21658: clang-format: [JS] handle conditionals in fields, default params.

[Daniel Jasper via cfe-commits]

djasper added inline comments.


Comment at: lib/Format/TokenAnnotator.cpp:1001
@@ -1000,3 +1000,3 @@
   if (Style.Language == FormatStyle::LK_JavaScript &&
-  Line.MustBeDeclaration) {
+  !Contexts.back().IsExpression) {
 // In JavaScript, `interface X { foo?(): bar; }` is an optional method

Well, if you keep the Line.MustBeDeclaration here, then the test case does not 
regress. I believe that the test in itself probably doesn't make much sense, 
but at least then we aren't regressing it. At any rate, it seems bad to keep 
the Line.MustBeDeclaration above and remove it here.


http://reviews.llvm.org/D21658



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