hokein added inline comments.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:169
-std::vector<Diag> generateMissingIncludeDiagnostics(
+using MissingIncludeEdits = llvm::MapVector<include_cleaner::Header, TextEdit>;
+MissingIncludeEdits generateMissingIncludeEdits(
----------------
kadircet wrote:
> what do we gain by preserving the insertion order here exactly?
>
> in `generateMissingIncludeDiagnostics`, we already traverse using the order
> of `MissingIncludeDiagInfo`s and inside `addAllMissingIncludes` the order we
> preserved here (diagnostic order) doesn't really make much sense, we probably
> should generate fixes in the order of insertion location, or header spelling.
>
> so what about just using a DenseMap ?
The reason was that we need a deterministic-order for visting
`MissingIncludeEdits`, otherwise that batch-fix lit test might fail from time
to time.
particularly, in `addAllMissingIncludes`, we generate a `Fix` by iterating
`MissingIncludeEdits`, we construct the `Fix.Edits`, and `Fix.Annotations`.
- For `Fix.Edits`, as you pointed out in another comment, we can sort it by
(range, newText), to make the case where multiple #includes insertion are at
the same place better;
- For `Fix.Annotations`, this is a bit hard to do a sort afterward as we
construct the ID + Annotation, but luckily the final order doesn't matter
there, we only need a deterministic order to make lit test happy.
The other alternative is to switch `MissingIncludeEdits` to a `vector<{Header,
TextEdit}>` sorted by the `TextEdit`, but in
`generateMissingIncludeDiagnostics`, we need to lookup from a `Header`, then
this means we have to do a linear scan which doesn't feel good.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:330
+ llvm::DenseMap<include_cleaner::Header,
+ llvm::SetVector<include_cleaner::Symbol>>
+ HeaderToSymbols;
----------------
kadircet wrote:
> we might still have symbols with the same name (`ns1::Foo` vs `ns2::Foo`),
> they'll show up the same in the final annotation.
>
> since we're already sorting by name, we might as well deduplicate after
> sorting and store just a SmallVector here
you're right, done.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:353
+ ChangeAnnotationIdentifier ID = AddAllMissingID + std::to_string(I++);
+ AddAllMissing.Edits.push_back(Edit);
+ AddAllMissing.Edits.back().annotationId = ID;
----------------
kadircet wrote:
> we might generate multiple insertions to same location here e.g. insert `a.h`
> and `b.h` at the top of the file.
> [LSP](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textEditArray)
> says that order reported by the server will be preserved. hence we should
> actually make sure our edits are ordered by spelling.
Good catch, this seems like an existing problem today. Sorting these edits by
spelling header is one solution,
I'm planning to do it in this patch, added a FIXME.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D149437/new/
https://reviews.llvm.org/D149437
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
