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