[PATCH] D58346: [Sema] Change addr space diagnostics in casts to follow C++ style

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

One comment, but otherwise LGTM.



================
Comment at: lib/Sema/SemaCast.cpp:2309
+    auto DestPointeeTypeWithoutAS = Self.Context.removeAddrSpaceQualType(
+        DestPointeeType.getCanonicalType());
+    return Self.Context.hasSameType(SrcPointeeTypeWithoutAS,
----------------
ebevhan wrote:
> Anastasia wrote:
> > Anastasia wrote:
> > > ebevhan wrote:
> > > > Maybe I'm mistaken, but won't getting the canonical type here drop qualifiers (like cv) in nested pointers? If so, an addrspace_cast might strip qualifiers by mistake.
> > > Yes, indeed I will need to extend this to nested pointers when we are ready. But for now I can try to change this bit... however I am not sure it will work w/o canonical types when we have typedef. I will try to create an example and see.
> > I checked the canonical type does preserve the qualifiers correctly.
> > 
> > Here is the AST dump of the following C type `mytype const __generic*` where  `typedef  __generic int* mytype;`.
> > 
> > 
> > ```
> > PointerType 0x204d3b0 'const __generic mytype *'
> > `-QualType 0x204d369 'const __generic mytype' const __generic
> >   `-TypedefType 0x204d320 'mytype' sugar
> >     |-Typedef 0x204d1b0 'mytype'
> >     `-PointerType 0x204d170 '__generic int *'
> >       `-QualType 0x204d158 '__generic int' __generic
> >         `-BuiltinType 0x2009750 'int'
> > ```
> > 
> > and it's canonical representation in AST is:
> > 
> > ```
> > PointerType 0x204d380 '__generic int *const __generic *'
> > `-QualType 0x204d349 '__generic int *const __generic' const __generic
> >   `-PointerType 0x204d170 '__generic int *'
> >     `-QualType 0x204d158 '__generic int' __generic
> >       `-BuiltinType 0x2009750 'int'
> > ```
> > 
> > So using canonical type will just simply handling of nested pointer chain by avoiding special casing typedefs. We won't loose any qualifiers.
> Okay, seems good then!
It seems to me that the rules in this function are the reasonable cross-language rules.  If you want to check for OpenCL at the top as a fast-path check (taking advantage of that fact that no other language modes currently have overlapping address spaces), that might be reasonable, but the comment and indentation should reflect that.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58346/new/

https://reviews.llvm.org/D58346