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

Reply via email to