[PATCH] D42895: [libclang] Add `CXSymbolRole role` to CXIdxEntityRefInfo

[Fangrui Song via Phabricator via cfe-commits]

MaskRay marked 4 inline comments as done.
MaskRay added inline comments.


================
Comment at: include/clang-c/Index.h:6159
+   */
+  CXSymbolRole role;
 } CXIdxEntityRefInfo;
----------------
ilya-biryukov wrote:
> MaskRay wrote:
> > ilya-biryukov wrote:
> > > 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`
> > 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
> 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.
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.


================
Comment at: tools/libclang/CXIndexDataConsumer.cpp:154
+  // CXSymbolRole is synchronized with clang::index::SymbolRole.
+  return CXSymbolRole(static_cast<uint32_t>(Role));
+}
----------------
ilya-biryukov wrote:
> MaskRay wrote:
> > ilya-biryukov wrote:
> > > `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,
> > > ```
> > 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.
> > 
> > 
> 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?
zeroed high bits of CXSymbolRole and left comments.


Repository:
  rC Clang

https://reviews.llvm.org/D42895