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:
> > > > > 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?
> I believe so, yes -- that would ensure it's the minimally desugared type
> which the type is canonically equivalent to.
Sorry for the holdup. So for an AddressSpace AttributedType, we attach the
qualifiers only to the equivalent type.
As for this though, the only problem I ran into with returning the equivalent
type is for AttributedTypes with `attr::ObjCKindOf`, a test expects returning
the modified type which is an `ObjCInterface` instead of the equivalent type
which is an `ObjCObject`. The test itself just tests printing of a type, but
I'm not sure how significant or intentional printing this specific way was.
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
