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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
