This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Use `ObjCProtocolLoc` for generalized ObjC protocol support
ClosedPublic

Authored by dgoldman on Feb 9 2022, 12:30 PM.

Details

Summary

This removes clangd's existing workaround in favor of proper support
via the newly added ObjCProtocolLoc. This improves support by
allowing clangd to properly identify which protocol is selected
now that ObjCProtocolLoc gets its own ASTNode.

Diff Detail

Event Timeline

dgoldman created this revision.Feb 9 2022, 12:30 PM
dgoldman requested review of this revision.Feb 9 2022, 12:30 PM
dgoldman updated this revision to Diff 407287.Feb 9 2022, 2:04 PM

Minor lint fixes

sammccall accepted this revision.Feb 10 2022, 12:01 PM

LG, thanks!
You might want to check out how hover behaves and add a test there.

clang-tools-extra/clangd/unittests/FindTargetTests.cpp
948

This new test case looks interesting, but can we keep the old one too?

This revision is now accepted and ready to land.Feb 10 2022, 12:01 PM
dgoldman added inline comments.Feb 10 2022, 1:45 PM
clang-tools-extra/clangd/unittests/FindTargetTests.cpp
948

It now fails, I thought this was expected since the selection is now split up into multiple nodes - should it continue to work as it did previously?

llvm-project/clang-tools-extra/clangd/unittests/FindTargetTests.cpp:953: Failure
Value of: assertNodeAndPrintDecls("ObjCObjectTypeLoc")
Expected: has 1 element and that element is equal to @protocol Foo Rel=
  Actual: {}

    @protocol Foo
    @end
    void test([[id<Foo>]] p);
dgoldman updated this revision to Diff 407674.Feb 10 2022, 1:56 PM

add hover test

sammccall accepted this revision.Feb 10 2022, 2:07 PM
sammccall added inline comments.
clang-tools-extra/clangd/unittests/FindTargetTests.cpp
948

Oops, I wasn't reading carefully enough, you're right!

dgoldman updated this revision to Diff 408937.Feb 15 2022, 9:47 AM

Rebase on top of latest ProtocolLoc changes