kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:334
+const Decl *SymbolCollector::getRefContainer(const Decl *Enclosing) {
+ const NamedDecl *ND = dyn_cast_or_null<NamedDecl>(Enclosing);
----------------
why do we need to expose this function in the class interface? ASTCtx is
reachable from Decl, and we can just pass in Opts.
Also I believe we should take in a declcontext as an input parameter, and make
use of `ASTNode.ParentDC` for starting the traversal.
This also requires some comments explaining that it'll return the first context
that is known by index.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:335
+const Decl *SymbolCollector::getRefContainer(const Decl *Enclosing) {
+ const NamedDecl *ND = dyn_cast_or_null<NamedDecl>(Enclosing);
+ while (ND && !shouldCollectSymbol(*ND, *ASTCtx, Opts, true)) {
----------------
in theory enclosing namespace can also be unnamed, something like a `BlockDecl`.
I think we should enforce the return value to be a named decl, but I don't
think we should enforce any intermediate parents to be named decls.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:489
DeclRefs[ND].push_back(
- SymbolRef{SM.getFileLoc(Loc), Roles, ASTNode.Parent});
+ SymbolRef{SM.getFileLoc(Loc), Roles, getRefContainer(ASTNode.Parent)});
// Don't continue indexing if this is a mere reference.
----------------
as mentioned above I think we should start the traversal from
`ASTNode.ParentDC`. any downsides to doing that ? I don't see a case where this
container we return is **not** a `DeclContext`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D105083/new/
https://reviews.llvm.org/D105083
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
