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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
