leonardchan added inline comments.
================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6943 +def err_typecheck_incompatible_conditional_pointer_operands : Error< + "unable to find common type between %0 and %1 for conditional operation">; ---------------- ebevhan wrote: > This error is very similar to the one in my first comment, `conditional > operator with the second and third operands of type > ('__attribute__((address_space(1))) char *' and > '__attribute__((address_space(2))) char *') which are pointers to > non-overlapping address spaces`. It would normally be emitted at 6472, but > won't be since OpenCL isn't enabled. Done. Removing the check for OpenCL throws this instead of the warning. ================ Comment at: lib/Sema/SemaExpr.cpp:6522 + bool HasDifferingLAddrSpace = LAddrSpace != ResultAddrSpace; + bool HasDifferingRAddrSpace = RAddrSpace != ResultAddrSpace; + ---------------- rjmccall wrote: > I was going to tell you to use the predicate > `Qualifiers::isAddressSpaceSupersetOf` here, but then I was looking at the > uses of that, and I think the real fix is to just go into the implementation > of `checkConditionalPointerCompatibility` and make the compatibility logic > not OpenCL-specific. The fast-path should just be whether the address spaces > are different. > > And it looks like this function has a bug where it always uses > `LangAS::Default` outside of OpenCL even if the pointers are in the same > address space. I'm not sure how the `LangAS::Default`, but removing all checks for OpenCL does the trick and prints an existing error relating to different address_spaces on conditional operands to replace the warning. Only 2 tests needed the change from the expected warning to expected error without having to change any OpenCL tests. I also think the address_space comparison is already done with the `lhQual.isAddressSpaceSupersetOf` and `rhQual.isAddressSpaceSupersetOf`. ================ Comment at: test/Sema/conditional-expr.c:78 + // expected-error@-1{{converting '__attribute__((address_space(2))) int *' to type 'void *' changes address space of pointer}} + // expected-error@-2{{converting '__attribute__((address_space(3))) int *' to type 'void *' changes address space of pointer}} ---------------- ebevhan wrote: > rjmccall wrote: > > leonardchan wrote: > > > rjmccall wrote: > > > > Also, these diagnostics seem wrong. Where is `void *` coming from? > > > When dumping the AST this is what the resulting type is for the > > > conditional expression already is if the operands are 2 pointers with > > > different address spaces. > > > > > > According to this comment, the reason seems to be because this is what > > > GCC does: > > > > > > ``` > > > 6512 // In this situation, we assume void* type. No especially good > > > 6513 // reason, but this is what gcc does, and we do have to pick > > > 6514 // to get a consistent AST. > > > ``` > > That makes sense in general, but in this case it's not a great diagnostic; > > we should just emit an error when trying to pick a common type. > Is it possible that you are getting `void *` because we aren't running the > qualifier removal at 6495? I don't think I've ever seen spurious `void *`'s > show up in our downstream diagnostics. So the `void *` is what get's dumped for me using the latest upstream version of clang and is the result of the `ConditionalOperator`. An AST dump of ``` 3 unsigned long test0 = 5; 4 int __attribute__((address_space(2))) *adr2; 5 int __attribute__((address_space(3))) *adr3; 6 test0 ? adr2 : adr3; ``` for me returns ``` `-ConditionalOperator 0xbdbcab0 <line:6:3, col:18> 'void *' |-ImplicitCastExpr 0xbdbc690 <col:3> 'unsigned long' <LValueToRValue> | `-DeclRefExpr 0xbdbc618 <col:3> 'unsigned long' lvalue Var 0xbdbc348 'test0' 'unsigned long' |-ImplicitCastExpr 0xbdbc790 <col:11> 'void *' <BitCast> | `-ImplicitCastExpr 0xbdbc6a8 <col:11> '__attribute__((address_space(2))) int *' <LValueToRValue> | `-DeclRefExpr 0xbdbc640 <col:11> '__attribute__((address_space(2))) int *' lvalue Var 0xbdbc490 'adr2' '__attribute__((address_space(2))) int *' `-ImplicitCastExpr 0xbdbc7a8 <col:18> 'void *' <BitCast> `-ImplicitCastExpr 0xbdbc6c0 <col:18> '__attribute__((address_space(3))) int *' <LValueToRValue> `-DeclRefExpr 0xbdbc668 <col:18> '__attribute__((address_space(3))) int *' lvalue Var 0xbdbc5a0 'adr3' '__attribute__((address_space(3))) int *' ``` Repository: rC Clang https://reviews.llvm.org/D50278 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits