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