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