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
