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

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.


================
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.
----------------
kadircet wrote:
> 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`.
I assume you mean `ASTNode.ContainerDC`.

I tried this suggestion, but it looks like `ContainerDC` does not contain what we want in many cases.

For all of the following cases in `SymbolCollectorTest.RefContainers`:

 * ref2 (variable initializer)
 * ref3 (function parameter default value)
 * ref5 (template parameter default value)
 * ref6 (type of variable)
 * ref7a (return type of function)
 * ref7b (parameter type of function)

`ASTNode.ContainerDC` is the `TranslationUnitDecl` (i.e. the `DeclContext` in which the function or variable is declared) rather than the function or variable itself.

In addition, for ref4 (member initializer), `ContainerDC` is the `RecordDecl` rather than the `FieldDecl`.

So, I don't think this suggestion will work without making the `Ref::Container` field significantly less specific (and thereby the call hierarchy feature less useful in some cases).



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105083/new/

https://reviews.llvm.org/D105083