[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:6721
 
+  if (getLangOpts().OpenCL) {
+
----------------
Anastasia wrote:
> rjmccall wrote:
> > Anastasia wrote:
> > > rjmccall wrote:
> > > > Since you're moving this code anyway, can this be split into its own function?  I'm not sure if it's actually important that some of these failures return immediately and some of them fall through to later checks.
> > > >  I'm not sure if it's actually important that some of these failures return immediately and some of them fall through to later checks.
> > > 
> > > Yes, it looks a bit random. Do we have any guideline whether we should return or keep diagnosing as long as possible?
> > > 
> > > 
> > If the user is likely to have made multiple independent errors, it's good to diagnose as many of them as possible.  But if it's just as likely that the user messed up in a single way that should've meant we didn't take this code path, then it's better to bail out early.
> > 
> > In this case, most of these diagnostics are looking for different special OpenCL types, and those are probably all independent  of each other.
> > 
> > ...it does occur to me to wonder if more of these checks should be looking through array types the way that the check for `half` does.  Presumably you shouldn't be able to declare global arrays of images or pipes if you can't declare non-array variables of them.
> > ...it does occur to me to wonder if more of these checks should be looking through array types the way that the check for half does. Presumably you shouldn't be able to declare global arrays of images or pipes if you can't declare non-array variables of them.
> 
> We actually provide dedicated diagnostic for all other types in `Sema::BuildArrayType`. No idea why half is handled here I will try to refactor that in a separate patch. 
Well, `half` is a perfectly reasonable type to have an array of.  I don't know why that's not equally true of images or pipes; are they assumed to have implicit trailing storage?  Anyway, if OpenCL says arrays of them are forbidden, we don't need to look through arrays; that's probably worth a comment, though.


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

https://reviews.llvm.org/D65744