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