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

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.


================
Comment at: lib/Sema/SemaCast.cpp:2224
+  } else if (IsLValueCast) {
     Kind = CK_LValueBitCast;
   } else if (DestType->isObjCObjectPointerType()) {
----------------
Anastasia wrote:
> 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?
My last sentence could have been a bit clearer. Yes, for the `__private`->`__global` case, it should error. But I was thinking more of the case where the AS conversion is valid, like `__private`->`__generic`. Then we will do the AS conversion, but we should have done both an AS conversion and an `LValueBitCast`, because we need to both convert the 'pointer' and also dereference it.


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

https://reviews.llvm.org/D58346