This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Support for standard type hierarchy
ClosedPublic

Authored by kadircet on Aug 8 2022, 2:27 AM.

Details

Summary

This is mostly a mechanical change to adapt standard type hierarchy
support proposed in LSP 3.17 on top of clangd's existing extension support.

This does mainly two things:

  • Incorporate symbolids for all the parents inside resolution parameters, so that they can be retrieved from index later on. This is a new code path, as extension always resolved them eagerly.
  • Propogate parent information when resolving children, so that at least one branch of parents is always preserved. This is to address a shortcoming in the extension.

This doesn't drop support for the extension, but it's deprecated from now on and
will be deleted in upcoming releases. Currently we use the same struct
internally but don't serialize extra fields.

Diff Detail

Event Timeline

kadircet created this revision.Aug 8 2022, 2:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2022, 2:27 AM
Herald added a subscriber: arphaman. · View Herald Transcript
kadircet requested review of this revision.Aug 8 2022, 2:28 AM
Trass3r added a subscriber: Trass3r.Aug 8 2022, 2:37 AM
nridge added a subscriber: nridge.Aug 13 2022, 2:55 PM
usaxena95 added inline comments.Aug 16 2022, 1:26 AM
clang-tools-extra/clangd/Protocol.cpp
1228

Nit: Allow RVO.

clang-tools-extra/clangd/Protocol.h
1415

nit.

clang-tools-extra/clangd/XRefs.cpp
1953–1962

Can you revert the formatting changes in l:1894-1964

clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp
781

It would be clearer if we use getSymbolID(&findDecl(AST, "Parent1")) here and in SuperTypes.

kadircet marked 3 inline comments as done.Aug 16 2022, 2:21 AM
kadircet added inline comments.
clang-tools-extra/clangd/Protocol.cpp
1228

there's an issue with one of the compilers used by a build-bot which can't find the relevant constructor if we don't have std::move here (hence we've the same pattern across others). we should definitely check at some point to see if that compiler is gone, but i'd rather not do that in the scope of this patch.

clang-tools-extra/clangd/XRefs.cpp
1953–1962

oops, thanks for noticing.

clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp
781

not sure what you mean by also doing it in SuperTypes

kadircet updated this revision to Diff 452927.Aug 16 2022, 2:21 AM
kadircet marked 2 inline comments as done.
  • Address comments
usaxena95 accepted this revision.Aug 16 2022, 4:45 AM

LG. Thanks!

clang-tools-extra/clangd/Protocol.cpp
1228

Interesting. Makes sense to get to it later.

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

nit: std::move.

clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp
781

Nvm. I was referring to SuperTypes test but it does not check for the symbol id.

This revision is now accepted and ready to land.Aug 16 2022, 4:45 AM
kadircet marked 2 inline comments as done.Aug 17 2022, 12:29 AM
kadircet added inline comments.
clang-tools-extra/clangd/XRefs.cpp
1713

the copy here is actually intentional. as we need to preserve data both in current element and also in its parent.

This revision was landed with ongoing or failed builds.Aug 17 2022, 12:33 AM
This revision was automatically updated to reflect the committed changes.
kadircet marked an inline comment as done.