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

Leonard Chan via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Jan 15 13:05:53 PST 2019


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





More information about the cfe-commits mailing list