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

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo added a comment.

Thanks for the comments, see the updated version.



================
Comment at: clang-tools-extra/clangd/Hover.cpp:1173
+        if (Ref.RT != include_cleaner::RefType::Explicit ||
+            UsedSymbolNames.find(getRefName(Ref)) != UsedSymbolNames.end())
+          return;
----------------
kadircet wrote:
> 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`
Ok, I am storing `include_cleaner::Symbols` now (not the Decls, since those would require a switch on the symbol kind, etc.) and `llvm::sort`'ing the result then.


================
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:
> 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;
> }
> ```
```
for (auto *MatchingInc : MatchingIncludes) {
  if (MatchingInc->Line == Inc.HashLine + 1)
    UsedSymbolNames.insert(getRefName(Ref));
}
```
I'm not sure I agree that this snippet is better. It's more verbose than the current version, and IMO matching the hovered include against the provider is quite readable already. 

I'm using your suggestions for the comments now, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146244