ebevhan marked 2 inline comments as done.
ebevhan 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<
----------------
Anastasia wrote:
> ebevhan wrote:
> > Anastasia wrote:
> > > 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"
> > That is doable, but all of the 'typecheck' errors/warnings are made to be
> > regular. If I add another parameter, there needs to be a special case in
> > DiagnoseAssignmentResult for that error in particular.
> Oh I see... might not worth it?
I think keeping the generality makes it a bit simpler. Technically many of the
errors here could be folded into one or two instead, but that hasn't been done,
so...
================
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 **'}}
+}
----------------
Anastasia wrote:
> ebevhan wrote:
> > 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.
> > 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?
>
> Hmm, good point! C99 spec seems to be helpless. C++ seems to imply that it
> checks pointers left to right as far as I interpret that from [conv.qual].
> Not sure what we should do... Would it make sense to align with C++ or
> otherwise whatever is simpler? At least there is a diagnostic generated. So
> perhaps after all it's good enough for now!
>
>
> > 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.
>
> Ok, common helper function could be another solution to avoid duplication but
> it seems the logic is not entirely identical.
>
>
> Would it make sense to align with C++ or otherwise whatever is simpler?
I think C++ is a bit stricter in general, it doesn't permit this, but C is more
lenient. There is a diagnostic, so I left a FIXME but it should probably be
revisited.
> Ok, common helper function could be another solution to avoid duplication but
> it seems the logic is not entirely identical.
I added a new enum member and removed the logic on the error emission side.
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
