[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:443
+  RemoveAll.Annotations.push_back({RemoveAllUnusedID,
+                                   {/*label=*/"", /*needsConfirmation=*/true,
+                                    /*description=*/std::nullopt}});
----------------
hokein wrote:
> 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.
> 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).

i think you're focusing too much on the way VSCode chooses to implement this interaction.

Showing the diff in the edit panel is a strategy chosen by VSCode, not per LSP. Spec indicates `label` as `A human-readable string describing the actual change. The string is rendered prominent in the user interface.`. So there's a good chance an editor might chose to just display the `label`. This makes even more sense especially when changes are complicated and don't fit a single line.

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

Right, but we actually now have the opportunity to describe each fix properly. We can change the fix message we generate in `generateUnusedIncludeDiagnostics` to contain proper include and use them as annotation labels instead. We already have the logic inside Diagnostics.cpp to synthesize messages for fixes (this would also unify the behaviour), i think we should re-use it here by exposing it in diagnostics.h. WDYT?


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:463
+  // FIXME(hokein): emit used symbol reference in the annotation.
+  ChangeAnnotation Annotation = {/*label=*/"",
+                                 /*needsConfirmation=*/true,
----------------
similar to above, let's at least mention the fix properly in label.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:471
+    ChangeAnnotationIdentifier ID = AddAllMissingID + std::to_string(I++);
+    AddAllMissing.Edits.push_back(It.getValue());
+    AddAllMissing.Edits.back().annotationId.emplace(ID);
----------------
nit: you can `std::move(It.getValue())` if you drop the `const` from `It`.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:481
+  FixAll.Message = "fix all includes";
+  ChangeAnnotation Annotation = {/*label=*/"",
+                                 /*needsConfirmation=*/true,
----------------
why are we creating new annotations here? rather than just merging everything from `RemoveAllUnused` and `AddAllMissing` ?


================
Comment at: clang-tools-extra/clangd/Protocol.h:253
+  /// The actual annotation identifier.
+  std::optional<ChangeAnnotationIdentifier> annotationId = std::nullopt;
 };
----------------
despite keeping this one as-is, we still don't need to optional, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147684