[PATCH] D153769: [clangd] Implement the 'Organize Imports' source action. Fix include-cleaner findings in batch.

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo added a comment.

Thanks for the comments!



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:62
+  const auto &SM = Inputs.AST->getSourceManager();
+  std::set<std::string> Spellings;
+  for (const auto &Missing : Findings.MissingIncludes)
----------------
kadircet wrote:
> prefer `llvm::StringSet<>`
I think it should actually be `llvm::DenseSet<inlude_cleaner::Header>`. We should first de-duplicate providers and only then spell the resulting headers. In this version, we were potentially spelling the same provider multiple times.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:64-67
+    for (const auto &P : Missing.Providers)
+      Spellings.insert(include_cleaner::spellHeader(
+          {P, Inputs.AST->getPreprocessor().getHeaderSearchInfo(),
+           SM.getFileEntryForID(SM.getMainFileID())}));
----------------
kadircet wrote:
> this will insert all providers for a symbol (e.g. if we have both `foo.h` and `bar.h`, we'll insert both).
you're right, thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153769