[PATCH] D146244: [clangd] Show used symbols on #include line hover.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1173
+        if (Ref.RT != include_cleaner::RefType::Explicit ||
+            UsedSymbolNames.find(getRefName(Ref)) != UsedSymbolNames.end())
+          return;
----------------
creating these symbol names for every reference is still a lot of waste, you can directly cache `Ref.Target` pointers, which are a lot cheaper. you can also store them in an `llvm::DenseSet<const Decl*>` (`std::set` has O(logN) operation times). afterwards you can call `getRefName` only once, on the decls that we care about, and llvm::sort the result.

also because you're using just the declname, we might get erroneous de-duplication (symbols might have same name under different qualifiers) and all of a sudden `ns1::Foo` might suppress the analysis of `ns2::Foo` because logic here will think we've already seen a symbol named `Foo`


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1178
+          // Find the highest-rank #include'd provider
+          const auto &MatchingIncludes = ConvertedMainFileIncludes.match(H);
+          if (MatchingIncludes.empty())
----------------
you can't actually keep a reference here, return is a value type. just `auto MatchingIncludes = ..`;


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1178
+          // Find the highest-rank #include'd provider
+          const auto &MatchingIncludes = ConvertedMainFileIncludes.match(H);
+          if (MatchingIncludes.empty())
----------------
kadircet wrote:
> you can't actually keep a reference here, return is a value type. just `auto MatchingIncludes = ..`;
nit: this might read easier with:
```
auto MatchingIncludes = ConvertedMainFileIncludes.match(H);
// No match for this provider in the main file.
if (MatchingIncludes.empty())
  continue;
// Check if our include matched this provider.
for (auto *MatchingInc : MatchingIncludes) {
  if (MatchingInc->Line == Inc.HashLine + 1)
    UsedSymbolNames.insert(getRefName(Ref));
// Don't look for rest of the providers once we've found a match in the main file.
break;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146244