[PATCH] D147684: [clangd] Add batch fixes for include-cleaner diagnostics

[Kadir Cetinkaya via Phabricator via cfe-commits]

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