Page MenuHomePhabricator

[clangd] Add ObjC method support to prepareCallHierarchy
ClosedPublic

Authored by sheldonneuberger-sc on Tue, Nov 16, 10:32 PM.

Details

Summary

This fixes "textDocument/prepareCallHierarchy" in clangd for ObjC methods. Details at https://github.com/clangd/vscode-clangd/issues/247.

clangd uses Decl::isFunctionOrFunctionTemplate to check if the decl given in a prepareCallHierarchy request is eligible for prepareCallHierarchy. We change to use isFunctionOrMethod which includes functions and ObjC methods.

Diff Detail

Event Timeline

sheldonneuberger-sc edited the summary of this revision. (Show Details)Tue, Nov 16, 10:50 PM
sheldonneuberger-sc published this revision for review.Tue, Nov 16, 11:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Nov 16, 11:24 PM

Thanks for the patch.

Could you add a test exercising call hierarchy for obj-c methods to CallHierarchyTests.cpp please?

In terms of the actual change, this function has some other callers in Parser and Sema code, and I don't know how this change will affect them. It may be better to limit the change to the clangd call site for now (that is, change that call site to use a local helper function, which checks isFunctionOrFunctionTemplate() || <obj-c stuff>), and defer changing Decl::isFunctionOrFunctionTemplate() itself to a subsequent refactor.

I agree with Nathan on this one. It's unclear why there are two functions that does the same thing in different ways and some usages in sema looks suspicious enough to require caution (there are subtle checks for shadowing and whatnot and I don't know how these rules come into play in objc contexts).

So I would suggest updating clangd side to look like:

(isa<DeclContext>(Decl) && cast<DeclContext>(Decl)->isFunctionOrMethod()) || Decl->getlKind() == FunctionTemplate
  • move change to clangd callsite
  • add tests
Herald added a project: Restricted Project. · View Herald TranscriptWed, Nov 17, 8:31 PM
Herald added a subscriber: arphaman. · View Herald Transcript

Thanks for the detailed comments. I moved the patch into the clangd callsite. I also added a couple ObjC tests for CallHierarchy (I basically parameterized two of the existing tests).

sheldonneuberger-sc retitled this revision from [clang] Add ObjC decls to Decl::isFunctionOrFunctionTemplate to [clangd] Add ObjC method support to prepareCallHierarchy.Wed, Nov 17, 9:00 PM
sheldonneuberger-sc edited the summary of this revision. (Show Details)

Thanks, LGTM!

I have a couple of nits about the test changes, but with those I think this is good to merged.

clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
54

nit: rather than passing std::string by value, let's use llvm::StringRef

(const std::string& would also be fine, but I think llvm::StringRef is more conventional in this codebase)

177

nit: the intendation here seems a bit inconsistent. Can we either indent everything between @implementation and @end, or alternatively un-indent just this line?

kadircet added inline comments.Tue, Nov 23, 9:21 AM
clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
54

i am not sure if these helpers makes tests easier to read (i believe they're actually making it harder).

please inline them back. if we want to have better infrastructure for testing this, we need to do it in its own patch (and I definitely agree that we need a better infra for testing here. but we need to think about something that can generalize rather than forcing a particular pattern).

  • inline test helper for readability
  • fix indentation
  • revert whitespace change

Made suggested changes, ready for another review.

kadircet accepted this revision.Tue, Nov 23, 11:08 PM

thanks, lgtm! let me know of your email address (for commit attribution) if you want me to land this for you.

This revision is now accepted and ready to land.Tue, Nov 23, 11:08 PM

thanks, lgtm! let me know of your email address (for commit attribution) if you want me to land this for you.

While I don't see it surfaced anywhere in the Phabricator UI, there is in fact an email address associated with this changeset, and it shows up in the commit created by arc patch D114058.

This revision was automatically updated to reflect the committed changes.