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