[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType

[Aaron Ballman via Phabricator via cfe-commits]

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