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

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added inline comments.


================
Comment at: clang/lib/Format/SortJavaScriptImports.cpp:158
     std::string ReferencesText;
-    bool SymbolsInOrder = true;
-    for (unsigned i = 0, e = Indices.size(); i != e; ++i) {
-      JsModuleReference Reference = References[Indices[i]];
-      if (appendReference(ReferencesText, Reference))
-        SymbolsInOrder = false;
+    for (unsigned i = 0, e = References.size(); i != e; ++i) {
+      JsModuleReference Reference = References[i];
----------------
nit: capitalize, `I`, `E` (:


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


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


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


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


================
Comment at: clang/unittests/Format/SortImportsTestJS.cpp:362
+             "\n"
+             "export {B} from 'foo';\n",
              "import {A} from 'foo';\n"
----------------
Was this change intended? It doesn't seem related to this patch.


================
Comment at: clang/unittests/Format/SortImportsTestJS.cpp:385
+             "import {A} from './a';\n"
+             "// clang-format on\n");
+
----------------
can we have a test that checks that we don't merge import lines in an off section


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101515