This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Call hierarchy (Protocol layer)
ClosedPublic

Authored by nridge on Oct 12 2020, 11:31 PM.

Diff Detail

Event Timeline

nridge created this revision.Oct 12 2020, 11:31 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 12 2020, 11:31 PM
nridge retitled this revision from [clangd] Implement call hierarchy (incoming callers) to [clangd] Implement call hierarchy (incoming calls).Oct 12 2020, 11:36 PM
nridge updated this revision to Diff 298933.Oct 18 2020, 10:58 PM
  • Rebased on top of upstream changes
  • Rebased on top of TestWorkspace changes
  • Merged D89298 into this patch
  • Split index changes out to D89670
  • Added more tests

Keeping in the draft state for now, as I still need to address this comment from the issue discussion:

The request still has a progressToken attached to it, I presume(it is unclear from the spec) it is preserved between prepare and subsequent requests. So we can keep a mapping in ClangdServer and stash symbolIDs.

nridge updated this revision to Diff 300548.Oct 25 2020, 11:34 AM

Add and use CallHierarchyItem.data

nridge published this revision for review.Oct 25 2020, 11:36 AM

Keeping in the draft state for now, as I still need to address this comment from the issue discussion:

The request still has a progressToken attached to it, I presume(it is unclear from the spec) it is preserved between prepare and subsequent requests. So we can keep a mapping in ClangdServer and stash symbolIDs.

The protocol has since been amended to introduce a data field to CallHierarchyItem in which we can stash the SymbolID, making this workaround unnecessary.

The updated patch uses the data field, and should be ready for review.

nridge added a comment.EditedOct 25 2020, 11:39 AM

Note, testing this with vscode requires this vscode-clangd patch applied (to use a newer version of the client libraries that support the data field).

Kadir, would you like to take a look at this, as you're already familiar with the index changes? :)

Thanks, I skimmed over it (again) and I think this looks pretty great in general. I couldn't take a look at the details yet, I would really appreciate it if you could chop this one into smaller pieces (i know, i asked it before, but this is still too big). I suppose something like:

  • Changes in the XRefs side, introducing {prepare,incoming,outgoing} CallHierarchy bits, relevant Protocol.h additions and unittests for those.
  • Surfacing this in ClangdServer and ClangdLSPServer layer, with relevant unit/lit tests. (I would be even more happier if you could split this in two again one for each layer :) )

would be great.

If you can't split it up, that's also fine. I'll go over it eventually, it will just take longer and probably be harder to remember where we left off after each iteration :D

nridge updated this revision to Diff 304045.Nov 9 2020, 9:52 PM

Split patch as requested

nridge retitled this revision from [clangd] Implement call hierarchy (incoming calls) to [clangd] Call hierarchy (Protocol layer).Nov 9 2020, 9:53 PM
nridge edited the summary of this revision. (Show Details)
kadircet added inline comments.Nov 10 2020, 1:46 AM
clang-tools-extra/clangd/Protocol.cpp
1174

we should also populate tags and detail conditionally. rather than providing empty strings/vectors.

1203

s/"from"/"to"

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

we use exactly the same names in protocol here, even if they don't adhere to the style guide. i.e. s/Name/name. same for others.

1367

s/Rng/range

1376

i would drop the optional, as there's no difference between empty vs none.

1405

could you put this before CallHierarchyItem ?

1408

could you put this before CallHierarchyIncomingCall, same for outgoing

nridge updated this revision to Diff 305397.Nov 15 2020, 4:36 PM
nridge marked 7 inline comments as done.

Address review comments

nridge updated this revision to Diff 305406.Nov 15 2020, 7:52 PM

Forgot to rename a few fields

kadircet accepted this revision.Nov 17 2020, 1:50 AM

thanks, LGTM

This revision is now accepted and ready to land.Nov 17 2020, 1:50 AM
This revision was landed with ongoing or failed builds.Nov 18 2020, 12:42 AM
This revision was automatically updated to reflect the committed changes.