kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.
thanks, lgtm!
sorry for the late reply :( adding a couple nits.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:170
+ while (Enclosing) {
+ const NamedDecl *ND = dyn_cast_or_null<NamedDecl>(Enclosing);
+ if (ND && SymbolCollector::shouldCollectSymbol(*ND, ND->getASTContext(),
----------------
not need for `_or_null` as loop condition ensures `Enclosing != nullptr`
nit: `const auto *ND`
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:173
+ Opts, true)) {
+ return ND;
+ }
----------------
nit: I'd just break here and `return Enclosing` outside.
================
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:
> > 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).
> > 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 [...] 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)?
>
> In my opinion, in cases where a reference to a function occurs in the
> initializer (or otherwise in the declaration, such as perhaps in the type) of
> a namespace-scope variable, it is significantly more useful to show the name
> of that variable in the call hierarchy, than the name of the containing file.
>
> Consider an example like this:
>
> ```
> void FlushAndClearCaches();
> ...
> // elsewhere, at namespace scope:
> std::function<void()> OnMemoryPressureCallback = &FlushAndClearCaches;
> ```
>
> If I invoke call hierarchy on `FlushAndClearCaches` (or one of its callees),
> and I'm looking at the children of the `FlushAndClearCaches` node in the call
> hierarchy tree, it is much more informative if the name of the relevant child
> item is `OnMemoryPressureCallback`, than if it is a file name. If it's a file
> name, I need to take the extra step of clicking on the item to navigate to
> the target file and the reference location inside it, and look there, whereas
> if it's a variable name I may have all the information I need just from the
> call hierarchy view.
>
> I think being able to eyeball the call hierarchy view and get and idea of
> what sorts of functionality calls into a function, without having to click on
> each item and examine the context, is an important use case.
>
> (It's fair to ask if this logic also applies to local variables. I would say
> no: local variables are more likely to be implementation details with
> un-interesting names. The name of the containing function is typically the
> more interesting part, and is still more granular than a file.)
>
> I appreciate that on a technical level, this is an inconsistency between
> local variables vs. namespace-scope variables. However, from the point of
> view of a user of call hierarchy, I think this combination gives the best
> results.
>
> I am also open to revising what we store in `Ref::Container` if additional
> consumers (other than call hierarchy) come up, but for now I think it makes
> sense to let it be guided by what it useful for call hierarchy. (I surveyed
> the Eclipse CDT codebase, where the corresponding piece of information in the
> index has one other consumer besides call hierarchy: it's used to annotate
> entries in the find-references view with the name of the calling function
> (https://github.com/clangd/clangd/issues/177). Were we to add this feature, I
> think we'd want to keep the same treatment of variables as for call
> hierarchy, for similar reasons.)
> (It's fair to ask if this logic also applies to local variables. I would say
> no: local variables are more likely to be implementation details with
> un-interesting names. The name of the containing function is typically the
> more interesting part, and is still more granular than a file.)
Right, I was mostly saying that if we want this behaviour at one place, why we
don't want it at the other. I'd claim that top-level function declaration is
also going to be an implementation detail most of the time.
> I appreciate that on a technical level, this is an inconsistency between
> local variables vs. namespace-scope variables. However, from the point of
> view of a user of call hierarchy, I think this combination gives the best
> results.
SG. I suppose we can revisit the decision in the future if need be and store
the vardecl if need be and make callhiearachy iterate over the parents instead.
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
