This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Don't navigate to forward class declaration when go to definition.
ClosedPublic

Authored by hokein on Jan 2 2018, 2:01 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

hokein created this revision.Jan 2 2018, 2:01 AM
sammccall accepted this revision.Jan 2 2018, 4:54 AM
sammccall added inline comments.
clangd/XRefs.cpp
54 ↗(On Diff #128399)

Can you add some motivation here? e.g. the forward decl example

This revision is now accepted and ready to land.Jan 2 2018, 4:54 AM
hokein updated this revision to Diff 128501.Jan 3 2018, 2:12 AM
hokein marked an inline comment as done.
  • Revise the way of checking definition.
  • Add a test for function declaration.
hokein added a comment.Jan 3 2018, 2:14 AM

With more test, it turns out that ASTNode.OrigD is not always pointed to the definition, so we can't rely on D or ASTNode.OrigD :(.

I revised the way of checking definition, it should works for major cases. Please review it again.

ilya-biryukov added inline comments.Jan 3 2018, 2:33 AM
clangd/XRefs.cpp
68 ↗(On Diff #128501)

That seems like a useful helper on its own, maybe create a helper called Decl* getDefinition(Decl* D) and use it instead?
It's implementation can be as short as the one we currently have for IsDefinition, since all interesting Decl types have getDefinition method.

hokein updated this revision to Diff 128516.Jan 3 2018, 4:25 AM

Use getDefinition to simply the code.

hokein added inline comments.Jan 3 2018, 4:26 AM
clangd/XRefs.cpp
68 ↗(On Diff #128501)

Good point!

This revision was automatically updated to reflect the committed changes.