[PATCH] D61663: [clang-format] Fix a JavaScript import order bug.

[Owen Pan via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG265ed6819409: [clang-format] Fix a JavaScript import order 
bug (authored by owenpan).

Changed prior to commit:
  https://reviews.llvm.org/D61663?vs=557879=557888#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61663

Files:
  clang/lib/Format/SortJavaScriptImports.cpp
  clang/unittests/Format/SortImportsTestJS.cpp


Index: clang/unittests/Format/SortImportsTestJS.cpp
===
--- clang/unittests/Format/SortImportsTestJS.cpp
+++ clang/unittests/Format/SortImportsTestJS.cpp
@@ -514,6 +514,14 @@
  "export {Y};\n");
 }
 
+TEST_F(SortImportsTestJS, TemplateKeyword) {
+  // Reproduces issue where importing "template" disables imports sorting.
+  verifySort("import {template} from './a';\n"
+ "import {b} from './b';\n",
+ "import {b} from './b';\n"
+ "import {template} from './a';");
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: clang/lib/Format/SortJavaScriptImports.cpp
===
--- clang/lib/Format/SortJavaScriptImports.cpp
+++ clang/lib/Format/SortJavaScriptImports.cpp
@@ -548,10 +548,12 @@
   nextToken();
   if (Current->is(tok::r_brace))
 break;
-  bool isTypeOnly =
-  Current->is(Keywords.kw_type) && Current->Next &&
-  Current->Next->isOneOf(tok::identifier, tok::kw_default);
-  if (!isTypeOnly && !Current->isOneOf(tok::identifier, tok::kw_default))
+  auto IsIdentifier = [](const auto *Tok) {
+return Tok->isOneOf(tok::identifier, tok::kw_default, 
tok::kw_template);
+  };
+  bool isTypeOnly = Current->is(Keywords.kw_type) && Current->Next &&
+IsIdentifier(Current->Next);
+  if (!isTypeOnly && !IsIdentifier(Current))
 return false;
 
   JsImportedSymbol Symbol;
@@ -565,7 +567,7 @@
 
   if (Current->is(Keywords.kw_as)) {
 nextToken();
-if (!Current->isOneOf(tok::identifier, tok::kw_default))
+if (!IsIdentifier(Current))
   return false;
 Symbol.Alias = Current->TokenText;
 nextToken();


Index: clang/unittests/Format/SortImportsTestJS.cpp
===
--- clang/unittests/Format/SortImportsTestJS.cpp
+++ clang/unittests/Format/SortImportsTestJS.cpp
@@ -514,6 +514,14 @@
  "export {Y};\n");
 }
 
+TEST_F(SortImportsTestJS, TemplateKeyword) {
+  // Reproduces issue where importing "template" disables imports sorting.
+  verifySort("import {template} from './a';\n"
+ "import {b} from './b';\n",
+ "import {b} from './b';\n"
+ "import {template} from './a';");
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: clang/lib/Format/SortJavaScriptImports.cpp
===
--- clang/lib/Format/SortJavaScriptImports.cpp
+++ clang/lib/Format/SortJavaScriptImports.cpp
@@ -548,10 +548,12 @@
   nextToken();
   if (Current->is(tok::r_brace))
 break;
-  bool isTypeOnly =
-  Current->is(Keywords.kw_type) && Current->Next &&
-  Current->Next->isOneOf(tok::identifier, tok::kw_default);
-  if (!isTypeOnly && !Current->isOneOf(tok::identifier, tok::kw_default))
+  auto IsIdentifier = [](const auto *Tok) {
+return Tok->isOneOf(tok::identifier, tok::kw_default, tok::kw_template);
+  };
+  bool isTypeOnly = Current->is(Keywords.kw_type) && Current->Next &&
+IsIdentifier(Current->Next);
+  if (!isTypeOnly && !IsIdentifier(Current))
 return false;
 
   JsImportedSymbol Symbol;
@@ -565,7 +567,7 @@
 
   if (Current->is(Keywords.kw_as)) {
 nextToken();
-if (!Current->isOneOf(tok::identifier, tok::kw_default))
+if (!IsIdentifier(Current))
   return false;
 Symbol.Alias = Current->TokenText;
 nextToken();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61663: [clang-format] Fix a JavaScript import order bug.

[Owen Pan via Phabricator via cfe-commits]

owenpan updated this revision to Diff 557879.
owenpan added a comment.

Make the test case pass.


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

https://reviews.llvm.org/D61663

Files:
  clang/lib/Format/SortJavaScriptImports.cpp
  clang/unittests/Format/SortImportsTestJS.cpp


Index: clang/unittests/Format/SortImportsTestJS.cpp
===
--- clang/unittests/Format/SortImportsTestJS.cpp
+++ clang/unittests/Format/SortImportsTestJS.cpp
@@ -514,6 +514,14 @@
  "export {Y};\n");
 }
 
+TEST_F(SortImportsTestJS, TemplateKeyword) {
+  // Reproduces issue where importing "template" disables imports sorting.
+  verifySort("import {template} from './a';\n"
+ "import {b} from './b';\n",
+ "import {b} from './b';\n"
+ "import {template} from './a';\n");
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: clang/lib/Format/SortJavaScriptImports.cpp
===
--- clang/lib/Format/SortJavaScriptImports.cpp
+++ clang/lib/Format/SortJavaScriptImports.cpp
@@ -548,10 +548,12 @@
   nextToken();
   if (Current->is(tok::r_brace))
 break;
-  bool isTypeOnly =
-  Current->is(Keywords.kw_type) && Current->Next &&
-  Current->Next->isOneOf(tok::identifier, tok::kw_default);
-  if (!isTypeOnly && !Current->isOneOf(tok::identifier, tok::kw_default))
+  auto IsIdentifier = [](const auto *Tok) {
+return Tok->isOneOf(tok::identifier, tok::kw_default, 
tok::kw_template);
+  };
+  bool isTypeOnly = Current->is(Keywords.kw_type) && Current->Next &&
+IsIdentifier(Current->Next);
+  if (!isTypeOnly && !IsIdentifier(Current))
 return false;
 
   JsImportedSymbol Symbol;
@@ -565,7 +567,7 @@
 
   if (Current->is(Keywords.kw_as)) {
 nextToken();
-if (!Current->isOneOf(tok::identifier, tok::kw_default))
+if (!IsIdentifier(Current))
   return false;
 Symbol.Alias = Current->TokenText;
 nextToken();


Index: clang/unittests/Format/SortImportsTestJS.cpp
===
--- clang/unittests/Format/SortImportsTestJS.cpp
+++ clang/unittests/Format/SortImportsTestJS.cpp
@@ -514,6 +514,14 @@
  "export {Y};\n");
 }
 
+TEST_F(SortImportsTestJS, TemplateKeyword) {
+  // Reproduces issue where importing "template" disables imports sorting.
+  verifySort("import {template} from './a';\n"
+ "import {b} from './b';\n",
+ "import {b} from './b';\n"
+ "import {template} from './a';\n");
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: clang/lib/Format/SortJavaScriptImports.cpp
===
--- clang/lib/Format/SortJavaScriptImports.cpp
+++ clang/lib/Format/SortJavaScriptImports.cpp
@@ -548,10 +548,12 @@
   nextToken();
   if (Current->is(tok::r_brace))
 break;
-  bool isTypeOnly =
-  Current->is(Keywords.kw_type) && Current->Next &&
-  Current->Next->isOneOf(tok::identifier, tok::kw_default);
-  if (!isTypeOnly && !Current->isOneOf(tok::identifier, tok::kw_default))
+  auto IsIdentifier = [](const auto *Tok) {
+return Tok->isOneOf(tok::identifier, tok::kw_default, tok::kw_template);
+  };
+  bool isTypeOnly = Current->is(Keywords.kw_type) && Current->Next &&
+IsIdentifier(Current->Next);
+  if (!isTypeOnly && !IsIdentifier(Current))
 return false;
 
   JsImportedSymbol Symbol;
@@ -565,7 +567,7 @@
 
   if (Current->is(Keywords.kw_as)) {
 nextToken();
-if (!Current->isOneOf(tok::identifier, tok::kw_default))
+if (!IsIdentifier(Current))
   return false;
 Symbol.Alias = Current->TokenText;
 nextToken();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61663: [clang-format] Fix a JavaScript import order bug.

[Owen Pan via Phabricator via cfe-commits]

owenpan commandeered this revision.
owenpan edited reviewers, added: bowenni; removed: owenpan.
owenpan added a comment.

The test case doesn't pass.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61663

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

[PATCH] D61663: [clang-format] Fix a JavaScript import order bug.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.
Herald added a project: All.
Herald added reviewers: rymiel, HazardyKnusperkeks, owenpan, MyDeveloperDay.

@MyDeveloperDay this looks like it still could be relevant.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61663

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

[PATCH] D61663: [clang-format] Fix a JavaScript import order bug.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Are you waiting for someone to land this?


Repository:
  rC Clang

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

https://reviews.llvm.org/D61663



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

[PATCH] D61663: [clang-format] Fix a JavaScript import order bug.

[Daniel Jasper via Phabricator via cfe-commits]

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

Generally, upload patches with the full file as context (that will prevent 
Phabricator's "Context not available")
But this change looks good. Thank you.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61663



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

[PATCH] D61663: [clang-format] Fix a JavaScript import order bug.

[Bowen Ni via Phabricator via cfe-commits]

bowenni created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
bowenni retitled this revision from "Fix a JavaScript import order bug." to 
"[clang-format] Fix a JavaScript import order bug.".

When the imported symbol is exactly "template" the sorting is disabled.


Repository:
  rC Clang

https://reviews.llvm.org/D61663

Files:
  clang/lib/Format/SortJavaScriptImports.cpp
  clang/unittests/Format/SortImportsTestJS.cpp


Index: clang/unittests/Format/SortImportsTestJS.cpp
===
--- clang/unittests/Format/SortImportsTestJS.cpp
+++ clang/unittests/Format/SortImportsTestJS.cpp
@@ -307,6 +307,14 @@
  "import {A} from 'a';\n");
 }
 
+TEST_F(SortImportsTestJS, TemplateKeyword) {
+  // Reproduces issue where importing "template" disables imports sorting.
+  verifySort("import {template} from './a';\n"
+ "import {b} from './b';\n",
+ "import {b} from './b';\n"
+ "import {template} from './a';\n");
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: clang/lib/Format/SortJavaScriptImports.cpp
===
--- clang/lib/Format/SortJavaScriptImports.cpp
+++ clang/lib/Format/SortJavaScriptImports.cpp
@@ -409,7 +409,7 @@
   nextToken();
   if (Current->is(tok::r_brace))
 break;
-  if (!Current->isOneOf(tok::identifier, tok::kw_default))
+  if (!Current->isOneOf(tok::identifier, tok::kw_default, 
tok::kw_template))
 return false;
 
   JsImportedSymbol Symbol;


Index: clang/unittests/Format/SortImportsTestJS.cpp
===
--- clang/unittests/Format/SortImportsTestJS.cpp
+++ clang/unittests/Format/SortImportsTestJS.cpp
@@ -307,6 +307,14 @@
  "import {A} from 'a';\n");
 }
 
+TEST_F(SortImportsTestJS, TemplateKeyword) {
+  // Reproduces issue where importing "template" disables imports sorting.
+  verifySort("import {template} from './a';\n"
+ "import {b} from './b';\n",
+ "import {b} from './b';\n"
+ "import {template} from './a';\n");
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: clang/lib/Format/SortJavaScriptImports.cpp
===
--- clang/lib/Format/SortJavaScriptImports.cpp
+++ clang/lib/Format/SortJavaScriptImports.cpp
@@ -409,7 +409,7 @@
   nextToken();
   if (Current->is(tok::r_brace))
 break;
-  if (!Current->isOneOf(tok::identifier, tok::kw_default))
+  if (!Current->isOneOf(tok::identifier, tok::kw_default, tok::kw_template))
 return false;
 
   JsImportedSymbol Symbol;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits