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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia marked an inline comment as done.
Anastasia added inline comments.


================
Comment at: lib/Sema/SemaCast.cpp:2309
+    auto DestPointeeTypeWithoutAS = Self.Context.removeAddrSpaceQualType(
+        DestPointeeType.getCanonicalType());
+    return Self.Context.hasSameType(SrcPointeeTypeWithoutAS,
----------------
rjmccall wrote:
> 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.
Btw, I attempted to remove the OpenCL check and unfortunately found failure in the following test: **test/CodeGenCXX/address-space-cast.cpp**


```
error: C-style cast from 'char *' to '__attribute__((address_space(5))) char *' converts between mismatching address spaces
  __private__ char *priv_char_ptr = (__private__ char *)gen_char_ptr;
```

I am not sure what such conversion does but I feel if I want to update it I might need to get wider agreement. I think sending an RFC might be a good way forward.



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

https://reviews.llvm.org/D58346