leonardchan marked an inline comment as done. leonardchan added inline comments.
================ Comment at: clang/tools/libclang/CXType.cpp:132 + if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes) && + ATT->getAttrKind() != attr::AddressSpace) { return MakeCXType(ATT->getModifiedType(), TU); ---------------- aaron.ballman wrote: > leonardchan wrote: > > aaron.ballman wrote: > > > leonardchan wrote: > > > > aaron.ballman wrote: > > > > > This change seems surprising -- if the parsing options say the caller > > > > > does not want attributed types, why are we returning one anyway for > > > > > address space? > > > > This has to do with ensuring `clang_getAddressSpace` still returns the > > > > proper address_space. It does this by essentially checking the > > > > qualifiers of the type, which we now attach to the `AttributedType` > > > > whereas before it was attached to the modified type. > > > > > > > > This extra condition is necessary for ensuring that calling > > > > `clang_getAddressSpace` points to the qualified AttributedType instead > > > > of the unqualified modified type. > > > My fear is that this will be breaking assumptions in third-party code. If > > > someone disables `CXTranslationUnit_IncludeAttributedTypes`, they are > > > unlikely to expect to receive an `AttributedType` and may react poorly to > > > it. > > Instead check if the type is address_space attributed and apply the > > qualifiers the modified type. > Sure, they can always modify their code to handle it gracefully, but it's a > silent, breaking change to a somewhat stable API which is why I was prodding > about it. > > After talking with @rsmith, we're thinking the correct change here is to > return the attributed type when the user asks for it, but return the > equivalent type rather than the modified type if the user doesn't want > attribute sugar (for all attributes, not just address spaces). This way, when > a user asks for CXType for one of these, they always get a correct type but > sometimes it has attribute type sugar and sometimes it doesn't, depending on > the parsing options. > > This is still a silent, breaking change in behavior, which is unfortunate. It > should probably come with a mention in the release notes about the change to > the API and some unit tests as well. Ok. In the case of qualifiers then, should the equivalent type still contain the address_space qualifiers when creating the AttributedType? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55447/new/ https://reviews.llvm.org/D55447 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits