ilya-biryukov added inline comments.
================ Comment at: include/clang-c/Index.h:6159 + */ + CXSymbolRole role; } CXIdxEntityRefInfo; ---------------- 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. ================ Comment at: tools/libclang/CXIndexDataConsumer.cpp:154 + // CXSymbolRole is synchronized with clang::index::SymbolRole. + return CXSymbolRole(static_cast<uint32_t>(Role)); +} ---------------- 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? 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
