kadircet added inline comments.
================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:418 +Fix removeAllUnusedIncludes(llvm::ArrayRef<const Inclusion *> UnusedIncludes) { + Fix RemoveAll; ---------------- can we also derive these from an `llvm::ArrayRef<Diag>` ? to make sure there can't be a discrepancy between sum of these and individual unused include fixes ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:420 + Fix RemoveAll; + RemoveAll.Message = "remove all unused #includes"; + for (const auto *Inc : UnusedIncludes) { ---------------- i think we should drop `#` (same for `add all` case) ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:434 + assert(Diag.Fixes.size() == 1 && "Expected exactly one fix."); + AddAllMissing.Edits.insert(AddAllMissing.Edits.end(), + Diag.Fixes.front().Edits.begin(), ---------------- rather than copying all the fixes and then sorting/deleting them, can we have a map here to make sure we're copying only 1 edit per `Edit::newText` ? ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:451 + llvm::StringRef Code) { + Fix RemoveAllUnused = removeAllUnusedIncludes(Findings.UnusedIncludes); + ---------------- what if there's none? (same for add all) ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:458 + Fix FixAll; + FixAll.Message = "add all missing #includes and remove all unused ones"; + FixAll.Edits = RemoveAllUnused.Edits; ---------------- again fixall shouldn't be attached if either of the sets is empty ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:458 + Fix FixAll; + FixAll.Message = "add all missing #includes and remove all unused ones"; + FixAll.Edits = RemoveAllUnused.Edits; ---------------- kadircet wrote: > again fixall shouldn't be attached if either of the sets is empty this is quite wordy, what about just `fix includes` ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147684/new/ https://reviews.llvm.org/D147684 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits