[PATCH] D23972: clang-format: [JS] Sort all JavaScript imports if any changed.

[Martin Probst via cfe-commits]

mprobst created this revision.
mprobst added a reviewer: djasper.
mprobst added a subscriber: cfe-commits.
Herald added a subscriber: klimek.

User feedback is that they expect *all* imports to be sorted if any import was
affected by a change, not just imports up to the first non-affected line, as
clang-format currently does.

https://reviews.llvm.org/D23972

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

Index: unittests/Format/SortImportsTestJS.cpp
===================================================================
--- unittests/Format/SortImportsTestJS.cpp
+++ unittests/Format/SortImportsTestJS.cpp
@@ -190,40 +190,29 @@
 }
 
 TEST_F(SortImportsTestJS, AffectedRange) {
-  // Sort excluding a suffix.
-  verifySort("import {sym} from 'b';\n"
+  // Affected range inside of import statements.
+  verifySort("import {sym} from 'a';\n"
+             "import {sym} from 'b';\n"
              "import {sym} from 'c';\n"
-             "import {sym} from 'a';\n"
+             "\n"
              "let x = 1;",
              "import {sym} from 'c';\n"
              "import {sym} from 'b';\n"
              "import {sym} from 'a';\n"
              "let x = 1;",
              0, 30);
-  // Sort excluding a prefix.
+  // Affected range outside of import statements.
   verifySort("import {sym} from 'c';\n"
-             "import {sym} from 'a';\n"
-             "import {sym} from 'b';\n"
-             "\n"
-             "let x = 1;",
-             "import {sym} from 'c';\n"
              "import {sym} from 'b';\n"
              "import {sym} from 'a';\n"
              "\n"
              "let x = 1;",
-             30, 0);
-  // Sort a range within imports.
-  verifySort("import {sym} from 'c';\n"
-             "import {sym} from 'a';\n"
-             "import {sym} from 'b';\n"
-             "import {sym} from 'c';\n"
-             "let x = 1;",
              "import {sym} from 'c';\n"
              "import {sym} from 'b';\n"
              "import {sym} from 'a';\n"
-             "import {sym} from 'c';\n"
+             "\n"
              "let x = 1;",
-             24, 30);
+             70, 1);
 }
 
 TEST_F(SortImportsTestJS, SortingCanShrink) {
Index: lib/Format/SortJavaScriptImports.cpp
===================================================================
--- lib/Format/SortJavaScriptImports.cpp
+++ lib/Format/SortJavaScriptImports.cpp
@@ -284,14 +284,8 @@
     SourceLocation Start;
     bool FoundLines = false;
     AnnotatedLine *FirstNonImportLine = nullptr;
+    bool AnyImportAffected = false;
     for (auto Line : AnnotatedLines) {
-      if (!Line->Affected) {
-        // Only sort the first contiguous block of affected lines.
-        if (FoundLines)
-          break;
-        else
-          continue;
-      }
       Current = Line->First;
       LineEnd = Line->Last;
       skipComments();
@@ -309,6 +303,7 @@
         FirstNonImportLine = Line;
         break;
       }
+      AnyImportAffected = AnyImportAffected || Line->Affected;
       Reference.Range.setEnd(LineEnd->Tok.getEndLoc());
       DEBUG({
         llvm::dbgs() << "JsModuleReference: {"
@@ -325,6 +320,9 @@
       References.push_back(Reference);
       Start = SourceLocation();
     }
+    // Sort imports if any import line was affected.
+    if (!AnyImportAffected)
+      References.clear();
     return std::make_pair(References, FirstNonImportLine);
   }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D23972.69511.patch
Type: text/x-patch
Size: 2964 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160828/62ec5de8/attachment-0001.bin>