[PATCH] D149437: [clangd] Emit ChangeAnnotation label and description for include-cleaner diagnostics.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:445
 
-Fix removeAllUnusedIncludes(llvm::ArrayRef<Diag> UnusedIncludes) {
+Fix removeAllUnusedIncludes(llvm::ArrayRef<Diag> UnusedIncludes,
+                            llvm::StringRef Code) {
----------------
what about just taking in `llvm::ArrayRef<const Inclusion *> UnusedIncludes` and having a `TextEdit dropInclude(const Inclusion&);` that we can use in here & inside `generateUnusedIncludeDiagnostics`? That way we can just use the spelling inside the inclusion to generate the `label`, without all the extra logic.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:485
   // newText (#include "my_missing_header.h") -> TextEdit.
-  llvm::StringMap<TextEdit> Edits;
+  llvm::StringMap<std::pair<TextEdit, const MissingIncludeDiag*>> Edits;
   for (const auto &Diag : MissingIncludeDiags) {
----------------
i think rather than a single diag info, we should actually store a set of them now, right? as we want to surface not only the "first" use of a header, but all. (possibly with some trimming).


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:514
+             /*description=*/
+             llvm::formatv("Line {0}: '{1}'", StartLine, Lines[StartLine])}});
   }
----------------
i don't think line information is as useful (as there's no structured link to it). what about something like:

```
label: `Include "foo.h"`
description: `Provides 'X', 'Y', 'Z', ...`
```


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:550
   std::optional<Fix> FixAll;
   if (RemoveAllUnused && AddAllMissing)
     FixAll = fixAll(*RemoveAllUnused, *AddAllMissing);
----------------
sorry for missing it during last review, but this should actually be `if (!UnusedIncludes.empty() && !MissingIncludeDiags.empty())` as even if we have only 1 fix for each category, we should still show the fix all. probably it deserves a fix in a separate patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149437/new/

https://reviews.llvm.org/D149437