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

[Martin Probst via Phabricator via cfe-commits]

mprobst marked 9 inline comments as done.
mprobst added a comment.

PTAL, addressed review comments.



================
Comment at: clang/lib/Format/SortJavaScriptImports.cpp:191
+        !(FirstNonImportLine->First->is(tok::comment) &&
+          FirstNonImportLine->First->TokenText.trim() == "// clang-format on"))
       ReferencesText += "\n";
----------------
krasimir wrote:
> note that we generally also support `/* clang-format off/on */`: https://github.com/llvm/llvm-project/blob/aaf026d9da3885a951dcdc5edd64c8e7d23b6285/clang/lib/Format/Format.cpp#L2390
We don't in the C++ import sorter. Sigh.

I think the Real Fix (tm) would be folding the clang-format off thing in import sorting with affected lines, wouldn't it? I don't quite know how to untangle this, but it seems suboptimal that each sorter here re-implements the same logic.


================
Comment at: clang/lib/Format/SortJavaScriptImports.cpp:251
+  // references that have formatting enabled in individual chunks.
+  SmallVector<JsModuleReference, 16>
+  sortModuleReferences(SmallVector<JsModuleReference, 16> &References) {
----------------
krasimir wrote:
> We should pass in and return `llvm::SmallVectorImpl<JsModuleReference>` (the size version is mainly for local vars). Also, take the arg by `const` reference.
That gives me:

`error: no viable conversion from 'SmallVector<[...], 16>' to 'const SmallVector<[...], (default) CalculateSmallVectorDefaultInlinedElements<T>::value aka 1>'`


================
Comment at: clang/lib/Format/SortJavaScriptImports.cpp:288
+  // longer needed references).
+  void mergeModuleReferences(SmallVector<JsModuleReference, 16> &References) {
+    if (References.empty())
----------------
krasimir wrote:
> use `llvm::SmallVectorImpl<JsModuleReference>`
As above, this seems not possible.


================
Comment at: clang/lib/Format/SortJavaScriptImports.cpp:377
+      }
       skipComments();
       if (Start.isInvalid() || References.empty())
----------------
h-joo wrote:
> 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?
Nice, good catch! Done.


================
Comment at: clang/unittests/Format/SortImportsTestJS.cpp:362
+             "\n"
+             "export {B} from 'foo';\n",
              "import {A} from 'foo';\n"
----------------
krasimir wrote:
> Was this change intended? It doesn't seem related to this patch.
the empty line is intended. Previously this wouldn't end up formatting because the code was trying to be too clever in detecting changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101515