This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Implement textDocument/typeDefinition
ClosedPublic

Authored by sammccall on Dec 31 2021, 7:00 AM.

Details

Summary

This reuses the type=>decl mapping from go-to-definition on auto.
(Which could stand some improvement, but that can happen later).

Fixes https://github.com/clangd/clangd/issues/367

Diff Detail

Event Timeline

sammccall created this revision.Dec 31 2021, 7:00 AM
sammccall requested review of this revision.Dec 31 2021, 7:01 AM
usaxena95 accepted this revision.Jan 13 2022, 4:39 AM

Thanks. LGTM.

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

Reason for reverse isn't clear to me. Can you add a comment.

1880–1882

Can you add tests for these as well.

1883

I think this would be useful for enumerations primarily. Would it work as of now (would we return the enum definition loc) ?

1925–1927

Can this be accommodated in typeForNode ?
It would be nicer if we keep this method only for plumbing.

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

This is sounds confusing to me about the current behaviour.
We would return unique_ptr<T> here atm. Maybe just have simple returns Foo here as of now.

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

Can you add a lambda as well
"au^to x = []() -> Target {return Target{};}"

This revision is now accepted and ready to land.Jan 13 2022, 4:39 AM
sammccall marked 6 inline comments as done.Jan 13 2022, 1:14 PM
sammccall added inline comments.
clang-tools-extra/clangd/XRefs.cpp
1883

Yes, it does work (tested manually).

1925–1927

Agree. It's not convenient to do it exactly in typeForNode though, as we have to wrap every return with the unwrapping logic.
I added unwrapFindType to do this, and that's a natural place to fix array/pointer cases too while here.

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

Whoops, you're right. I'd love to have this behavior but decided not to bite it off in this patch.

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

Done. This isn't handled yet (lambdas are CXXRecordType, not FunctionType) so added it

This revision was automatically updated to reflect the committed changes.
sammccall marked 4 inline comments as done.
kadircet added inline comments.Sep 14 2022, 1:03 AM
clang-tools-extra/clangd/ClangdLSPServer.cpp
1292

any particular reason for jumping to declaration here, rather than using definition when it's available or was this just an oversight?

because this results in us jumping to forward declarations every now and then, and I can't see any use case where a declaration would be preferred to definition of a type.

Herald added a project: Restricted Project. · View Herald TranscriptSep 14 2022, 1:03 AM