Anastasia added inline comments.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6996
+ "|%diff{casting $ to type $|casting between types}0,1}2"
+ " changes address space of nested pointer">;
def err_typecheck_incompatible_ownership : Error<
----------------
I am wondering if we could just unify with the diagnostic above?
We could add another select at the end:
" changes address space of %select{nested|}3 pointer"
================
Comment at: lib/Sema/SemaCast.cpp:2294
+
+ // FIXME: C++ might want to emit different errors here.
if (Self.getLangOpts().OpenCL) {
----------------
I think my patch is divorcing C++ from here https://reviews.llvm.org/D58346.
Although, I might need to update it to add the same checks. I can do it once
your patch is finalized. :)
================
Comment at: lib/Sema/SemaCast.cpp:2295
+ // FIXME: C++ might want to emit different errors here.
if (Self.getLangOpts().OpenCL) {
+ const Type *DestPtr, *SrcPtr;
----------------
ebevhan wrote:
> I'd also like to petition to remove the guard on OpenCL here. Maybe an RFC
> for formalizing the support for address space conversion semantics is in
> order?
Yes, I'd support that! It would be good to generalize the rules as much as we
can rather than adding extra checks for languages (the semantic is very
similar).
As a matter of fact I tried to do the same in https://reviews.llvm.org/D58346
in `TryAddressSpaceCast` and some C++ tests fail. So I had to add OpenCL check
for now. :( Perhaps, they could just be changed but needs agreement with the
community first.
================
Comment at: lib/Sema/SemaExpr.cpp:14199
+ // qualifiers.
+ // XXX: Canonical types?
+ const Type *SrcPtr = SrcType->getPointeeType().getTypePtr();
----------------
May be, since if we are using a typedef that is a pointer type
`isPointerType()` might not return true? I would just extend a test case with
typedef to a pointer type as one of the nested levels.
================
Comment at: test/CodeGenOpenCL/numbered-address-space.cl:17
-void test_numbered_as_to_builtin(__attribute__((address_space(42))) int
*arbitary_numbered_ptr, float src) {
- volatile float result = __builtin_amdgcn_ds_fmaxf(arbitary_numbered_ptr,
src, 0, 0, false);
-}
----------------
Does this not compile any more?
================
Comment at: test/SemaOpenCL/address-spaces.cl:89
+ __local int * __global * __private * lll;
+ lll = gg; // expected-warning {{incompatible pointer types assigning to
'__local int *__global **' from '__global int **'}}
+}
----------------
ebevhan wrote:
> This doesn't seem entirely correct still, but I'm not sure what to do about
> it.
Is it because `Sema::IncompatiblePointer` has priority? We might want to change
that. I think it was ok before because qualifier's mismatch was only a warning
but now with the address spaces we are giving an error. I wonder if adding a
separate enum item for address spaces (something like
`Sema::IncompatibleNestedPointerAddressSpace`) would simplify things.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58236/new/
https://reviews.llvm.org/D58236
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
