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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
