[PATCH] D105083: [clangd] Ensure Ref::Container refers to an indexed symbol

[Kadir Cetinkaya via Phabricator via cfe-commits]

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