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:
> > > > > > > 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.
> Which test was failing because of this?
clang/test/Index/print-type.m
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
