Page MenuHomePhabricator

[clangd] Include refs of base method in refs for derived method.
ClosedPublic

Authored by usaxena95 on Oct 3 2021, 11:35 PM.

Details

Summary

Addresses https://github.com/clangd/clangd/issues/881

Includes refs of base class method in refs of derived class method.
Previously we reported base class method's refs only for decl of derived
class method. Ideally this should work for all usages of derived class method.

Related patch:
https://github.com/llvm/llvm-project/commit/fbeff2ec2bc6e44b92931207b0063f83ff7a3b3a.

Diff Detail

Event Timeline

usaxena95 created this revision.Oct 3 2021, 11:35 PM
usaxena95 requested review of this revision.Oct 3 2021, 11:35 PM
nridge added a subscriber: nridge.Oct 4 2021, 12:02 AM
kadircet added inline comments.Oct 4 2021, 1:46 AM
clang-tools-extra/clangd/XRefs.cpp
1395

it's important to note that this doesn't only suppress xrefs behaviour outside of the declaration, but it also suppresses xrefs on parts of the declaration apart from the function's name (like override/virtual keywords). it's unfortunate that we don't have tests to explicitly point out that fact.
I am not sure why the decision was to exclude them in the first place, I am definitely in favor of the new behaviour (we already provide go-to-def on override keyword for example, not having xrefs on it seems surprising). I suppose @hokein can tell if there are any other concerns I am missing.

clang-tools-extra/clangd/unittests/XRefsTests.cpp
1995

can we instead turn this test into a multi point check? (e.g. check that references in virt^ual void fu^nc() over^ride; ... D->fu^nc(); are all the same?

hokein added inline comments.Oct 4 2021, 5:29 AM
clang-tools-extra/clangd/XRefs.cpp
1395

yeah, I think the initial design was to return all base-class references when xrefs is called at the *declaration* (our previous concern was that base-class refs might add a lot of noises to the final results, which might be not what users want). Now we have user complains, let's do it for all places (LG the change).

Re the behavior on override keyword, this is probably an oversight (xrefs was not considered when we extended go-to-def). The implementation of go-to-def is ad-hoc, and only works for override, and final keywords (due to the limitation of AST). not sure it is a really useful feature, for consistency, it would be nice to have it in xrefs. I think it can be addressed in a follow-up patch.

usaxena95 updated this revision to Diff 377111.Oct 5 2021, 1:13 AM
usaxena95 marked 3 inline comments as done.

Addressed comments.

kadircet accepted this revision.Oct 5 2021, 1:25 AM

thanks, lgtm!

clang-tools-extra/clangd/XRefs.cpp
1393–1394

note that there's a behaviour change here as well. not sure how often it matters in practice, but previously we would still get the overriden methods for a symbol even if we fail to generate symbolid for it. let's keep it the same.

clang-tools-extra/clangd/unittests/XRefsTests.cpp
1783–1789

nit: I'd add braces here, even though this is still a single statement, it spans multiple lines and becomes confusing.

This revision is now accepted and ready to land.Oct 5 2021, 1:25 AM
usaxena95 updated this revision to Diff 377114.Oct 5 2021, 1:41 AM
usaxena95 marked an inline comment as done.

Addressed comments.

usaxena95 marked an inline comment as done.Oct 5 2021, 1:43 AM
usaxena95 added inline comments.
clang-tools-extra/clangd/XRefs.cpp
1393–1394

Hmm. You are right. Even in getOverriddenMethods we find the symbolID *optionally* and we always traverse the complete type hierarchy. Let's do the same here for consistency.