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