This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Report only decl of overridding method in xref.
ClosedPublic

Authored by usaxena95 on Feb 1 2021, 12:23 PM.

Details

Summary

See: https://github.com/clangd/clangd/issues/668

struct A { virtual void foo(); };
struct B : A { void foo() override; };

Find refs on A::foo() will show:

  • decls of A::foo()
  • decls of B::foo()
  • refs to A::foo()
  • no refs to B::foo().

Diff Detail

Event Timeline

usaxena95 created this revision.Feb 1 2021, 12:23 PM
usaxena95 requested review of this revision.Feb 1 2021, 12:23 PM
usaxena95 updated this revision to Diff 320563.Feb 1 2021, 12:30 PM

Move Index query to more logical place.

usaxena95 edited the summary of this revision. (Show Details)Feb 1 2021, 12:31 PM
usaxena95 added a reviewer: hokein.
usaxena95 added a subscriber: sammccall.

This looks about right to me...
Unfortunately I landed ff4832dbff0ccf1fd29f726efe72fd1220cd645a and 1eb7fd089e2fcf3fe211f865b28e2fed12128c3f meanwhile so this will need a rebase :-(

The first of those patches introduces flags on returned refs to indicate which are decls/defs. I'd suggest adding an "override" flag and populating it for these results.
Hopefully we'll have a sensible way to expose these flags over LSP someday, meanwhile I think our internal embedders may like to surface that.

(I guess the bug includes the other side of this, where if the method is an override, calls to base methods are reported. Makes sense to split it into a separate patch...)

usaxena95 updated this revision to Diff 320673.Feb 1 2021, 9:50 PM

Rebase + Add OverriddenBy flag for such references.

This looks about right to me...
Unfortunately I landed ff4832dbff0ccf1fd29f726efe72fd1220cd645a and 1eb7fd089e2fcf3fe211f865b28e2fed12128c3f meanwhile so this will need a rebase :-(

The first of those patches introduces flags on returned refs to indicate which are decls/defs. I'd suggest adding an "override" flag and populating it for these results.

Thanks.

(I guess the bug includes the other side of this, where if the method is an override, calls to base methods are reported. Makes sense to split it into a separate patch...)

Yes. I plan to do that in a separate patch.

usaxena95 updated this revision to Diff 320683.Feb 1 2021, 11:02 PM

Print value of new OverriddenBy flag.

usaxena95 updated this revision to Diff 320685.Feb 1 2021, 11:08 PM

Remove unintended changes.

hokein accepted this revision.Feb 1 2021, 11:43 PM

Thanks, this looks great.

clang-tools-extra/clangd/XRefs.cpp
1387

I guess we should include the definition location as well?

clang-tools-extra/clangd/XRefs.h
86

nit: add some doc, I'd just name it override as this field indicates this is an occurrence overrides the base method.

This revision is now accepted and ready to land.Feb 1 2021, 11:43 PM
usaxena95 updated this revision to Diff 320737.Feb 2 2021, 3:48 AM
usaxena95 marked 2 inline comments as done.

Addressed comments.

This revision was landed with ongoing or failed builds.Feb 2 2021, 4:06 AM
This revision was automatically updated to reflect the committed changes.