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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks, mostly LG. I think we need to be a little careful when generating insertions for all the missing includes though.



================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:169
 
-std::vector<Diag> generateMissingIncludeDiagnostics(
+using MissingIncludeEdits = llvm::MapVector<include_cleaner::Header, TextEdit>;
+MissingIncludeEdits generateMissingIncludeEdits(
----------------
what do we gain by preserving the insertion order here exactly?

in `generateMissingIncludeDiagnostics`, we already traverse using the order of `MissingIncludeDiagInfo`s and inside `addAllMissingIncludes` the order we preserved here (diagnostic order) doesn't really make much sense, we probably should generate fixes in the order of insertion location, or header spelling.

so what about just using a DenseMap ?


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:176
   const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID());
+  const Config &Cfg = Config::current();
 
----------------
nit: maybe inline into the call site


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:330
+  llvm::DenseMap<include_cleaner::Header,
+                 llvm::SetVector<include_cleaner::Symbol>>
+      HeaderToSymbols;
----------------
we might still have symbols with the same name (`ns1::Foo` vs `ns2::Foo`), they'll show up the same in the final annotation.

since we're already sorting by name, we might as well deduplicate after sorting and store just a SmallVector here


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:353
+    ChangeAnnotationIdentifier ID = AddAllMissingID + std::to_string(I++);
+    AddAllMissing.Edits.push_back(Edit);
+    AddAllMissing.Edits.back().annotationId = ID;
----------------
we might generate multiple insertions to same location here e.g. insert `a.h` and `b.h` at the top of the file. [LSP](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textEditArray) says that order reported by the server will be preserved. hence we should actually make sure our edits are ordered by spelling.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.h:44
   }
+  include_cleaner::Header header() const {
+    assert(!Providers.empty());
----------------
can you put definition out-of-line ?


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h:71
 
+  std::string name() const;
+
----------------
// Unqualified name of the symbol


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149437