Anastasia added a comment.
I think it's hard to write this simpler as logic is quite complicated. But we
can definitely improve understanding with comments!
Btw, regarding tests to cover case 2, could we also add test with types in
which CV qualifiers differ. May be just at least one. No need to test every
possible combination.
================
Comment at: lib/AST/ASTContext.cpp:7613
@@ +7612,3 @@
+ if (getLangOpts().OpenCL) {
+ if (LHS.getUnqualifiedType() != RHS.getUnqualifiedType() ||
+ LQuals.getCVRQualifiers() != RQuals.getCVRQualifiers())
----------------
Do you think we should check the types here? I was thinking we should do the
check exactly as below apart from AS specific part.
================
Comment at: lib/Sema/SemaExpr.cpp:6168
@@ -6168,3 +6167,3 @@
QualType CompositeTy = S.Context.mergeTypes(lhptee, rhptee);
if (CompositeTy.isNull()) {
----------------
Could we instead add a comment explaining different cases with AS we can have
here i.e. 1(a-c)&2(a-c)!
And may be we could refer to each case by adding comments in code below.
================
Comment at: lib/Sema/SemaExpr.cpp:6175
@@ +6174,3 @@
+ if (S.getLangOpts().OpenCL) {
+ // OpenCL v1.1 s6.5 - A pointer to address space A can only be assigned
+ // to a pointer to the same address space A. Casting a pointer to address
----------------
Could we write it shorter? For example: Conversion between pointers to distinct
address spaces is disallowed...
================
Comment at: lib/Sema/SemaExpr.cpp:6220-6232
@@ -6186,7 +6219,15 @@
ResultTy = S.Context.getBlockPointerType(ResultTy);
- else
+ else {
+ auto ResultAddrSpace = ResultTy.getQualifiers().getAddressSpace();
+ LHSCastKind = lhQual.getAddressSpace() == ResultAddrSpace
+ ? CK_BitCast
+ : CK_AddressSpaceConversion;
+ RHSCastKind = rhQual.getAddressSpace() == ResultAddrSpace
+ ? CK_BitCast
+ : CK_AddressSpaceConversion;
ResultTy = S.Context.getPointerType(ResultTy);
+ }
- LHS = S.ImpCastExprToType(LHS.get(), ResultTy, CK_BitCast);
- RHS = S.ImpCastExprToType(RHS.get(), ResultTy, CK_BitCast);
+ LHS = S.ImpCastExprToType(LHS.get(), ResultTy, LHSCastKind);
+ RHS = S.ImpCastExprToType(RHS.get(), ResultTy, RHSCastKind);
return ResultTy;
----------------
There are still blocks in ObjC, we shouldn't change!
================
Comment at: lib/Sema/SemaExpr.cpp:6222-6227
@@ -6188,1 +6221,8 @@
+ auto ResultAddrSpace = ResultTy.getQualifiers().getAddressSpace();
+ LHSCastKind = lhQual.getAddressSpace() == ResultAddrSpace
+ ? CK_BitCast
+ : CK_AddressSpaceConversion;
+ RHSCastKind = rhQual.getAddressSpace() == ResultAddrSpace
+ ? CK_BitCast
+ : CK_AddressSpaceConversion;
ResultTy = S.Context.getPointerType(ResultTy);
----------------
pxli168 wrote:
> What will mergetypes return?
> It seems the LHS and RHS are compatibel here, and may be they did not need
> bitcast?
I think we always need a cast here, because types are not exactly the same at
this point!
================
Comment at: lib/Sema/SemaExpr.cpp:6231
@@ -6189,3 +6230,3 @@
- LHS = S.ImpCastExprToType(LHS.get(), ResultTy, CK_BitCast);
- RHS = S.ImpCastExprToType(RHS.get(), ResultTy, CK_BitCast);
+ LHS = S.ImpCastExprToType(LHS.get(), ResultTy, LHSCastKind);
+ RHS = S.ImpCastExprToType(RHS.get(), ResultTy, RHSCastKind);
----------------
Btw, could we do the same thing with {LHS/RHS}CastKind variables in abode code.
It looks cleaner I think.
================
Comment at: test/SemaOpenCL/condition-operator-cl2.0.cl:19
@@ +18,3 @@
+
+void test_conversion(global int *arg_glob, local int *arg_loc,
+ constant int *arg_const, private int *arg_priv,
----------------
May be we could rename it to test_ternary and also merge with
test/SemaOpenCL/address-spaces-conversions-cl2.0.cl?
================
Comment at: test/SemaOpenCL/condition-operator-cl2.0.cl:20
@@ +19,3 @@
+void test_conversion(global int *arg_glob, local int *arg_loc,
+ constant int *arg_const, private int *arg_priv,
+ generic int *arg_gen, global char *arg_glob_ch,
----------------
Too many arguments. Could we move some into local variables instead?
================
Comment at: test/SemaOpenCL/condition-operator-cl2.0.cl:33
@@ +32,3 @@
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands
of type ('__constant int *' and '__local int *') which are pointers to
non-overlapping address spaces}}
+#elif defined(GLOBAL)
----------------
You can combine these two errors into 1 using regex. See examples in function
test_conversion of test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
================
Comment at: test/SemaOpenCL/condition-operator-cl2.0.cl:57
@@ +56,3 @@
+
+ void *var_void_gen;
+ constant void*var_void_const;
----------------
Could you declare just before the line using it?
Repository:
rL LLVM
http://reviews.llvm.org/D17412
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
