[PATCH] D147684: [clangd] Add batch fixes for include-cleaner diagnostics

[Kadir Cetinkaya via Phabricator via cfe-commits]

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