aaron.ballman 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);
----------------
leonardchan wrote:
> aaron.ballman wrote:
> > leonardchan wrote:
> > > leonardchan wrote:
> > > > 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
> > > Specifically just the check:
> > >
> > > ```
> > > CHECK: ParmDecl=p:6:36 (Definition) [type=__kindof Foo *]
> > > [typekind=ObjCObjectPointer] [canonicaltype=__kindof Foo *]
> > > [canonicaltypekind=ObjCObjectPointer] [basetype=Foo]
> > > [basekind=ObjCInterface] [isPOD=1] [pointeetype=Foo]
> > > [pointeekind=ObjCInterface]
> > > ```
> > >
> > > Where the last 2 fields we're instead `[pointeetype=__kindof Foo]
> > > [pointeekind=ObjCObject]` instead of what they are now.
> > I looked at the review adding the impacted test case and I did not have the
> > impression this was a principled decision so much as "that's what it
> > prints". I suspect we'd be fine removing the hack and always returning the
> > equivalent type, but ObjC is not my area of expertise.
> Done. Is there also a place I should add this to let others know of this
> change?
Yup, the release notes document (clang\docs\ReleaseNotes.rst) can be updated
with some words about the change in behavior.
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
