kadircet added a comment. thanks, mostly LG. I think we need to be a little careful when generating insertions for all the missing includes though.
================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:169 -std::vector<Diag> generateMissingIncludeDiagnostics( +using MissingIncludeEdits = llvm::MapVector<include_cleaner::Header, TextEdit>; +MissingIncludeEdits generateMissingIncludeEdits( ---------------- 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 ? ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:176 const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID()); + const Config &Cfg = Config::current(); ---------------- nit: maybe inline into the call site ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:330 + llvm::DenseMap<include_cleaner::Header, + llvm::SetVector<include_cleaner::Symbol>> + HeaderToSymbols; ---------------- 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 ================ 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; ---------------- 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. ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.h:44 } + include_cleaner::Header header() const { + assert(!Providers.empty()); ---------------- can you put definition out-of-line ? ================ Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h:71 + std::string name() const; + ---------------- // Unqualified name of the symbol 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
