kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:159
+// indexed, we walk further up because Ref::Container should always be
+// an indexed symbol.
+const Decl *getRefContainer(const Decl *Enclosing,
----------------
can you also add some info about why we are not using `DeclContext`s ?
something like:
```
Decls can nest under non-DeclContext nodes, in cases like initializer, where
they might be attributed to VarDecl.
Preserving that level of granularity is especially useful for initializers at
top-level, as otherwise the only context we
can attach these refs is TranslationUnitDecl, which we don't preserve in the
index.
FIXME: Maybe we should have some symbols for representing file-scopes, that way
we can show these refs as
being contained in the file-scope.
```
(Last fixme bit is optional, please add that if you also think the
functionality would be more useful that way)
================
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.
----------------
nridge wrote:
> 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).
>
> I assume you mean ASTNode.ContainerDC.
yes, sorry for the mess.
> 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).
I can't seem to remember why we went with it in the first place. But now:
```
int foo();
int x = foo();
void bar() {
int y = foo();
}
```
calling call hierarchy on `foo` will yield `x` and `bar`, creating some sort of
discrepancy. I am not really sure if telling first call to `foo` is contained
in `x` is really useful compared to saying it was contained in `filename.cpp`
(but ofc we can't do that today as container needs to be a symbol in the index,
hence the ref will be lost instead). do you also think that would be more
useful (i am not really a user of this feature so I would like to hear which is
better from some users)?
also I can't say that I find the discrepancy between an initializer in
parmvardecl/nontypetemplateparm vs vardecl useful (i know it already exists
today, but not really sure why).
they are actually attached to the first declcontext containing them, not the
first decl (which you say is more useful, let's leave the fact that they are
not indexed out for a while).
moreover it is a behavior we rely on through libindex, as it just marks
`ASTNode.Parent` with declcontext rather than making use of the vardecl. which
makes me believe it is not really important to have the top-level symbol as the
container. But it is the only way we can retain some container information.
if that's the case, I believe we should make this more explicit, i.e. start a
traversal from the node itself, remember the last indexed decl we've seen as a
parent and return that if we failed to find any indexed declcontext's
containing the decl (with a fixme saying that we could have some file symbols
in the index for translationunitdecls).
That way we can say that the container is the first `DeclContext` containing
the reference (with the exception of fixme mentioned).
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
