kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.
thanks for noticing this, LGTM!
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:961
const ParsedAST *
- const llvm::DenseSet<SymbolID> &TargetIDs;
+ llvm::DenseMap<const Decl *, SymbolID> TargetDeclAndID;
};
----------------
s/TargetDeclAndID/TargetDeclToID
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:965
std::vector<ReferenceFinder::Reference>
-findRefs(const llvm::DenseSet<SymbolID> &IDs, ParsedAST &AST, bool PerToken) {
- ReferenceFinder RefFinder(AST, IDs, PerToken);
+findRefs(const llvm::DenseSet<const Decl *> &TargetDecls, ParsedAST &AST,
+ bool PerToken) {
----------------
i'd actually update the interface here to just take an `ArrayRef<Decl*>` as
targets, as we're currently building those extra dense sets just to iterate
over them. both `allTargetDecls` and `getDeclAtPosition` (which uses
`allTargetDecls` underneath) are guaranteed to not return duplicates. So we can
pass their result directly here. Up to you though.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1919
// If there's a specific type alias, point at that rather than unwrapping.
- if (const auto* TDT = T->getAs<TypedefType>())
+ if (const auto *TDT = T->getAs<TypedefType>())
return QualType(TDT, 0);
----------------
can you revert this change?
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:951
if (const auto *Tok = TB.spelledTokenAt(L))
- References.push_back({*Tok, Roles, ID});
+ References.push_back({*Tok, Roles, getSymbolID(D)});
}
----------------
usaxena95 wrote:
> kadircet wrote:
> > we're still calling getsymbolid here lots of times, for probably the same
> > symbol.
> >
> > as discussed, what about building a lookup table on construction from
> > canonicaldecl to symbolid and use that lookup table instead of calling
> > getsymbolid over and over here?
> I don't mind adding a lookup table for this but this is not huge, only
> O(#ref) as compared to previous O(size of TU).
>
>
thanks. previous one was also size of main file, not the whole TU (if you see
indications of the latter in a different application let's take a look at that
separately).
but there are big enough files in which you can have thousands of references to
stringref and what not. so i think this actually matters.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1255
+ targetDecl(N->ASTNode, Relations, AST.getHeuristicResolver())) {
+ TargetDecls.insert(D);
+ }
----------------
usaxena95 wrote:
> kadircet wrote:
> > i mean directly inserting canonicaldecls here.
> This would make it messy to duplicate this on each end of the call.
> + This has potential of introducing bugs in future findRef calls if we don't
> pass the canonical decls. Or we would have to assert that we only get
> canonical decls.
> I think this is an implementation detail of findRefs and should not be
> visible outside its API for simplicity.
SG, it's an implementation detail of this file already so not a big deal. But
moreover we're building a separate lookup table already now, so obsolete.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125675/new/
https://reviews.llvm.org/D125675
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
