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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:443
+  RemoveAll.Annotations.push_back({RemoveAllUnusedID,
+                                   {/*label=*/"", /*needsConfirmation=*/true,
+                                    /*description=*/std::nullopt}});
----------------
kadircet wrote:
> rather than an empty label, let's duplicate `RemoveAll.Message` here. As that context will probably be lost after executing the code action.
I don't see much value on setting the label the the RemoveAll message (the preview window shows the content diff, it is clear enough for users to understand the purpose by viewing the diff).

And since we use one annotation per edit, the `RemoveAll` message doesn't seem to suit anymore.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:464
+      "AddAllMissingIncludes";
+  for (auto &E : AddAllMissing.Edits) {
+    E.annotationId.emplace();
----------------
kadircet wrote:
> i think you want to populate `AddAllMissing.Edits` from `Edits` first
oops, good catch!


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:468
+  }
+  // FIXME(hokein): emit used symbol reference in the annotation.
+  AddAllMissing.Annotations.push_back(
----------------
kadircet wrote:
> unless we want a really wordy description, this would actually require us to attach one annotation per edit, rather than using the same annotation for all the edits.
> can you check what changes in the UI when we have multiple annotations for a single workspace edit?
as discussed, there is not UI difference (at least in VSCode) between these two, but we should swich to one annotation per edit (since the UI is based on the annotation, and we want each edit is controlled by the user).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147684/new/

https://reviews.llvm.org/D147684