Page MenuHomePhabricator

[clangd] Fix irrelevant declaratations in goto definition (on macros).

Authored by hokein on Mar 9 2018, 2:01 AM.



DeclrationAndMacrosFinder will find some declarations (not macro!) that are
referened inside the macro somehow, isSearchedLocation() is not sufficient, we
don't know whether the searched source location is macro or not.

Diff Detail

Event Timeline

hokein created this revision.Mar 9 2018, 2:01 AM
ilya-biryukov added inline comments.Mar 12 2018, 5:29 AM

I wonder whether we should fix the DeclrationAndMacrosFinder to not return declarations coming from macro instantiations instead.
There are other clients (e.g. document highlights) that will probably break in the same way.

Do you think it would be possible and it would make sense for DeclrationAndMacrosFinder to only return a macro in this case and not return Decls coming from macro expansions?

hokein added inline comments.Mar 12 2018, 9:50 AM

I thought about it initially, but the information provided handleDeclOccurrence is limited...the occurrence location (FID and Offset) is expansion location (which is not always we want). That being said, when GoToDefinition on a macro, all declarations inside the macro body will be picked up.

In document hover implementation, we also use the same mechanism to avoid this problem :(

ilya-biryukov added inline comments.Mar 13 2018, 3:01 AM

As discussed offline, we should probably move this logic to DeclrationAndMacrosFinder so that all its clients (hover, documentHighlights, findDefinitions) are consistent.
Having a single function in DeclrationAndMacrosFinder that returns results (currently there are two: takeDecls() and takeMacros()) would allow that with minimal changes.

hokein updated this revision to Diff 138147.Mar 13 2018, 4:10 AM
hokein marked an inline comment as done.

Address review comment.


As discussed, made the workaround in Finder to keep the change minimal, no changes are required in client side.

ilya-biryukov added inline comments.Mar 13 2018, 5:20 AM

NIT: the fixme was a bit hard to follow for me. Maybe make it more clear where the problem should be handled, e.g.

FIXME: we should avoid adding decls from inside macros in handlDeclOccurence.
ilya-biryukov accepted this revision.Mar 13 2018, 5:20 AM

LGTM with a small nit about the comment.

This revision is now accepted and ready to land.Mar 13 2018, 5:20 AM
hokein updated this revision to Diff 138160.Mar 13 2018, 5:29 AM
hokein marked an inline comment as done.

Rephrase the fixme.

This revision was automatically updated to reflect the committed changes.