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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
