[PATCH] D58236: Make address space conversions a bit stricter.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan marked an inline comment as done.
ebevhan added inline comments.


================
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:
> Anastasia wrote:
> > 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.
> > Is it because `Sema::IncompatiblePointer` has priority?
> 
> Sort of. The problem is that the AS pointee qualifiers match up until the 'end' of the RHS pointer chain (LHS: `private->global->local`, RHS: `private->global`), so we never get an 'incompatible address space' to begin with. We only get that if 1) the bottommost type is equal after unwrapping pointers (as far as both sides go), or 2) any of the 'shared' AS qualifiers (as far as both sides go) were different.
> 
> The idea is that stopping when either side is no longer a pointer will produce 'incompatible pointers' when you have different pointer depths, but it doesn't consider anything below the 'shallowest' side of the pointer chain, so we miss out on any AS mismatches further down.
> 
> (Not that there's anything to mismatch, really. There is no matching pointer on the other side, so what is really the error?)
> 
> What should the criteria be for when the pointer types 'run out'? I could have it keep digging through the other pointer until it hits a different AS? This would mean that this:
> ```
> int **** a;
> int ** b = a;
> ```
> could give a different warning than it does today, though (incompatible nested qualifiers instead of incompatible pointers, which doesn't make sense...) . We would have to skip the `lhptee == rhptee` check if we 'kept going' despite one side not being a pointer type. So I don't know if that's the right approach in general.
> 
> Or should we be searching 'backwards' instead, starting from the innermost pointee? I don't know.
> 
> It really feels like the whole `checkPointerTypesForAssignment` routine and everything surrounding it is a bit messy. It relies on an implicit result from another function (`typesAreCompatible`) and then tries to deduce why that function thought the types weren't compatible. Then another function later on (`DiagnoseAssignmentResult`) tries to deduce why THIS function thought something was wrong.
> 
> > I wonder if adding a separate enum item for address spaces (something like `Sema::IncompatibleNestedPointerAddressSpace`) would simplify things.
> 
> This would simplify the logic on the error emission side, since we don't need to duplicate the logic for determining what went wrong, but doesn't help with diagnosing the actual problem. Probably a good idea to add it anyway, I just wanted to avoid adding a new enum member since that means updating a lot of code elsewhere.
> We only get that if 1) the bottommost type is equal after unwrapping pointers (as far as both sides go), or 2) any of the 'shared' AS qualifiers (as far as both sides go) were different.

Sorry, should only be 2) here. Was focused on the whole 'incompatible nested qualifiers' result.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58236/new/

https://reviews.llvm.org/D58236