[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:
> 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?
Thanks, the plan sounds good to me. As discussed, I will address it in a followup patch (added a FIXME here).


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147684