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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia marked 2 inline comments as done.
Anastasia added inline comments.


================
Comment at: lib/Sema/SemaCast.cpp:2224
+  } else if (IsLValueCast) {
     Kind = CK_LValueBitCast;
   } else if (DestType->isObjCObjectPointerType()) {
----------------
ebevhan wrote:
> This might not be applicable to this patch, but just something I noticed.
> 
> So `reinterpret_cast` only operates on pointers when dealing with address spaces. What about something like
> ```
> T a;
> T& a_ref = reinterpret_cast<T&>(a);
> ```
> `reinterpret_cast` on an lvalue like this is equivalent to `*reinterpret_cast<T*>(&a)`. So for AS code:
> ```
> __private T x;
> __generic T& ref = reinterpret_cast<__generic T&>(x);
> ```
> This should work, since `*reinterpret_cast<__generic T*>(&x)` is valid, correct?
> 
> What if we have the reference cast case with a different address space like this? Doesn't the `IsLValueCast` check need to be first?
> 
> What if we have the reference cast case with a different address space like this? Doesn't the IsLValueCast check need to be first?

Ok, let me see if I understand you. I changed `__generic` -> `__global` since it's invalid and the error is produced as follows:
  test.cl:7:21: error: reinterpret_cast from 'int' to '__global int &' is not allowed
  __global int& ref = reinterpret_cast<__global int&>(x);

Is this not what we are expecting here?


================
Comment at: lib/Sema/SemaCast.cpp:2309
+    auto DestPointeeTypeWithoutAS = Self.Context.removeAddrSpaceQualType(
+        DestPointeeType.getCanonicalType());
+    return Self.Context.hasSameType(SrcPointeeTypeWithoutAS,
----------------
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.


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

https://reviews.llvm.org/D58346