This is an archive of the discontinued LLVM Phabricator instance.

[libclang] Add `CXSymbolRole role` to CXIdxEntityRefInfo
ClosedPublic

Authored by MaskRay on Feb 4 2018, 12:16 PM.

Details

Summary

CXIdxEntityRefInfo contains the member CXIdxEntityRefKind kind; to
differentiate implicit and direct calls. However, there are more roles
defined in SymbolRole. Among them, Read/Write are probably the most
useful ones as they can be used to differentiate Read/Write occurrences
of a symbol for document highlight in a text document.

See export namespace DocumentHighlightKind
on https://microsoft.github.io/language-server-protocol/specification

Diff Detail

Event Timeline

MaskRay created this revision.Feb 4 2018, 12:16 PM
nridge added a subscriber: nridge.Feb 4 2018, 8:41 PM
yvvan added a comment.Feb 5 2018, 11:15 PM

I feel quite ok about this patch.

Can you please add unit-tests?

MaskRay updated this revision to Diff 133038.Feb 6 2018, 10:22 AM

Update c-index-test.c and clang/test/Index tests

I feel quite ok about this patch.

Can you please add unit-tests?

Added

// CHECK:      [indexEntityReference]: kind: field | name: y | {{.*}} | loc: 70:5 | {{.*}} | role: ref write
// CHECK:      [indexEntityReference]: kind: field | name: x | {{.*}} | loc: 70:11 | {{.*}} | role: ref read
// CHECK:      [indexEntityReference]: kind: function | name: foo3 | {{.*}} | loc: 71:10 | {{.*}} | role: ref addressof
// CHECK:      [indexEntityReference]: kind: function | name: foo4 | {{.*}} | loc: 72:3 | {{.*}} | role: ref call

to clang/test/Index/index-refs.cpp. I will implement read/write after this revision is landed.. https://github.com/cquery-project/cquery/issues/262

Looks ok-ish, I haven't built it though.
Also I don't have much exp with indexing part of libclang. Adding more reviewers.

ilya-biryukov added inline comments.Feb 7 2018, 9:36 AM
include/clang-c/Index.h
6163

Why do we need to store both CXIdxEntityRefKind and CXSymbolRole? Can we store just CXSymbolRole?
Is this for compatibility with existing clients?

If so, maybe we could:

  • remove Implicit and Direct from the CXSymbolRole
  • keep it only in CXIdxEntityRefKind
  • not deprecate the CXIdxEntityRefKind
tools/libclang/CXIndexDataConsumer.cpp
154

SymbolRoleSet seems to have more roles not covered by CXSymbolRole.
Should they be accepted by this function?
If yes, maybe zero those bits?
If no, maybe add an assert?

The extra roles are:

RelationChildOf     = 1 << 9,
RelationBaseOf      = 1 << 10,
RelationOverrideOf  = 1 << 11,
RelationReceivedBy  = 1 << 12,
RelationCalledBy    = 1 << 13,
RelationExtendedBy  = 1 << 14,
RelationAccessorOf  = 1 << 15,
RelationContainedBy = 1 << 16,
RelationIBTypeOf    = 1 << 17,
RelationSpecializationOf = 1 << 18,
MaskRay marked 2 inline comments as done.Feb 7 2018, 10:16 AM
MaskRay added inline comments.
include/clang-c/Index.h
6163

I think CXIdxEntityRefKind should be deprecated but for compatibility we do not remove the field for this minor version upgrade. But they can be removed after a major version upgrade.

For what I have observed, Implicit is only used by Objective-C

tools/libclang/CXIndexDataConsumer.cpp
154

Yes, it has more relations, but most are only used by Objective-C. In all test/Index tests, I have only seen RelationContainedBy used for .cpp files. Many have not occurred in .m files. So I do not want to expose them, at least for now.

MaskRay marked 2 inline comments as done.Feb 7 2018, 10:17 AM
ilya-biryukov added inline comments.Feb 8 2018, 3:36 AM
include/clang-c/Index.h
6163

We should definitely loop the owners of libclang in if we want to deprecate the API, I'm not familiar with the contract of libclang regarding the API deprecation.
I'm not sure who's responsible for that, asking at the cfe-dev mailing list could be your best bet for that.

The alternative I suggest is removing Implicit from CXSymbolRole, making this change an extension of existing API. I expect that this could be done without more through discussion.

tools/libclang/CXIndexDataConsumer.cpp
154

It's fine, but let's add a comment and zero those bits out.

Could you also a comments to both SymbolRoleSet and CXSymbolRole that they mirror each other and need to be updated together?

MaskRay updated this revision to Diff 133441.Feb 8 2018, 9:38 AM

Don't deprecate CXIdxEntityRefInfo

MaskRay marked 4 inline comments as done.Feb 8 2018, 9:41 AM
MaskRay added inline comments.
include/clang-c/Index.h
6163

I'll keep CXSymbolRole_Implicit and make it duplicate the functionality provided by CXIdxEntityRefKind, with a comment that CXIdxEntityRefKind may be deprecated in a future version.

Thanks, I'll also ask the cfe-dev mailing list.

tools/libclang/CXIndexDataConsumer.cpp
154

zeroed high bits of CXSymbolRole and left comments.

MaskRay marked 2 inline comments as done.Feb 8 2018, 10:09 AM
ilya-biryukov added inline comments.Feb 9 2018, 1:40 AM
include/clang-c/Index.h
6163

LG, thanks.

tools/c-index-test/c-index-test.c
3545

Maybe keep this debug-printing of refkind? We're not removing it in this patch.

MaskRay updated this revision to Diff 133632.Feb 9 2018, 9:24 AM

Bring back refkind:

MaskRay marked 4 inline comments as done.Feb 9 2018, 9:30 AM
MaskRay added a comment.EditedFeb 10 2018, 5:01 PM

Ping. Now this is a pure API extension to current implicit/direct roles. Is it possible to see this landed before clang+llvm 6 is released? This will be an amazing feature for LSP...

ilya-biryukov accepted this revision.Feb 12 2018, 2:12 AM

LGTM.

Is it possible to see this landed before clang+llvm 6 is released?

I don't know in detail how releases work, but I believe the release branch has already been created and this patch will need to be merged into it separately (i.e. landing this upstream won't get it into the release automatically).
But again, I'm not sure how releases work. Asking on cfe-dev or llvm-dev should be your best bet.

This revision is now accepted and ready to land.Feb 12 2018, 2:12 AM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.