kadircet added a comment.
can you also add test coverage for the new LSP fields?
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:439
+ for (auto &E : RemoveAll.Edits) {
+ E.annotationId.emplace();
+ *E.annotationId = RemoveAllUnusedID;
----------------
nit: `E.annotationId.emplace(RemoveAllUnusedID);` (same for others)
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:443
+ RemoveAll.Annotations.push_back({RemoveAllUnusedID,
+ {/*label=*/"", /*needsConfirmation=*/true,
+ /*description=*/std::nullopt}});
----------------
rather than an empty label, let's duplicate `RemoveAll.Message` here. As that
context will probably be lost after executing the code action.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:464
+ "AddAllMissingIncludes";
+ for (auto &E : AddAllMissing.Edits) {
+ E.annotationId.emplace();
----------------
i think you want to populate `AddAllMissing.Edits` from `Edits` first
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:468
+ }
+ // FIXME(hokein): emit used symbol reference in the annotation.
+ AddAllMissing.Annotations.push_back(
----------------
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?
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:484
+ FixAll.Annotations.push_back({FixAllID, {
+ /*label=*/"", /*needsConfirmation=*/true, /*description=*/std::nullopt
+ }});
----------------
again let's use `FixAll.Message` as label
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:493
+ llvm::StringRef Code) {
+ std::optional<Fix> RemoveAllUnused, AddAllMissing, FixAll;
+ std::vector<Diag> UnusedIncludes = generateUnusedIncludeDiagnostics(
----------------
can you declare these right before their use sides, rather than on the same
line?
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:496
+ AST.tuPath(), Findings.UnusedIncludes, Code);
+ if (!UnusedIncludes.empty())
+ RemoveAllUnused = removeAllUnusedIncludes(UnusedIncludes);
----------------
UnusedIncludes.size() > 1
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:501
+ AST, Findings.MissingIncludes, Code);
+ if (!MissingIncludeDiags.empty())
+ AddAllMissing = addAllMissingIncludes(MissingIncludeDiags);
----------------
MissingIncludeDiags.size() > 1
================
Comment at: clang-tools-extra/clangd/Protocol.h:253
+ /// The actual annotation identifier.
+ std::optional<ChangeAnnotationIdentifier> annotationId = std::nullopt;
};
----------------
rather than packing up a new field here, can we have a `struct
AnnotatedTextEdit : TextEdit` ?
it's surely more convenient this way but creates some confusion in call sites
that're trying to create a regular textedit.
also we can use the empty string again, no need for optional.
================
Comment at: clang-tools-extra/clangd/Protocol.h:264
+struct ChangeAnnotation {
+ // A human-readable string describing the actual change. The string
+ // is rendered prominent in the user interface.
----------------
nit: triple slashes, here and elsewhere
================
Comment at: clang-tools-extra/clangd/Protocol.h:274
+ // the user interface.
+ std::optional<std::string> description;
+};
----------------
no need for optional here, we can use the empty string
================
Comment at: clang-tools-extra/clangd/Protocol.h:1027
+ /// AnnotatedTextEdit.
+ std::optional<std::map<std::string, ChangeAnnotation>> changeAnnotations;
};
----------------
i don't think there's any difference between an empty map and a nullopt here,
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
