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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits