[PATCH] D101515: clang-format: [JS] handle "off" in imports

[Hana Joo via Phabricator via cfe-commits]

h-joo added inline comments.


================
Comment at: clang/lib/Format/SortJavaScriptImports.cpp:252
+  SmallVector<JsModuleReference, 16>
+  sortModuleReferences(SmallVector<JsModuleReference, 16> &References) {
+    // Sort module references.
----------------
`References` seem read-only. Can you make it `const Smallvector &`?


================
Comment at: clang/lib/Format/SortJavaScriptImports.cpp:257
+    // references per group.
+    auto *start = References.begin();
+    SmallVector<JsModuleReference, 16> ReferencesSorted;
----------------
nit : `Start`


================
Comment at: clang/lib/Format/SortJavaScriptImports.cpp:377
+      }
       skipComments();
       if (Start.isInvalid() || References.empty())
----------------
I think this has a potential bug that if the user makes
```
// clang-format off
// clang-format on
```
Every subsequent import will fail to be sorted. 

or maybe something like below
```
//clang-format off
import {A, B} from "./a"
//clang-format on
//clang-format off
```
will make the 'should not be sorted' part of the code be sorted.

Even if this is not the case, could you write a test for this one?


================
Comment at: clang/unittests/Format/SortImportsTestJS.cpp:405
+  // Boundary cases
+  verifySort("// clang-format on\n", "// clang-format on\n");
+  verifySort("// clang-format off\n", "// clang-format off\n");
----------------
Do you think it makes sense to add some weird test cases that // clang-format on-off switch, is pointless or ill-formed?
examples : 
```
// this is for the corner-case I mentioned above around the comment token handler
// clang-format off
// clang-format on
import {C} from "./.."
import {B} from "./.."
import {A} from "./.."
```

```
// The clang-format off comment might be dropped or not?
// clang-format on
import {C} from "./.."
import {B} from "./.."
import {A} from "./.."
// clang-format off
```

```
// This test maybe isn't necessary?
// clang-format off
// clang-format off
import {C} from "./.."
import {B} from "./.."
import {A} from "./.."
```

...and maybe some other fun examples! :) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101515