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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D65744#1652355 <https://reviews.llvm.org/D65744#1652355>, @Anastasia wrote:

> I don't think this is likely to change. Are you suggesting to move the deduction logic for pointee of pointers, references and block pointers into ASTContext helper that creates a pointer/reference/block pointer type?


No.  I'm suggesting that the deduction logic should be much more straightforward, just some sort of "is the type non-dependent and lacking a qualifier", and it should be applied in the two basic places we build these types in Sema, i.e. in the type-from-declarator logic and in the `Build{Pointer,Reference}Type` logic.  Instead we have something very elaborate that apparently recursively looks through pointer types and is contingent on the exact spelling, e.g. trying to find `auto` types, which seems both brittle and unnecessary.



================
Comment at: include/clang/AST/Type.h:6509
+  return isa<AutoType>(CanonicalType);
+}
+
----------------
Hmm.  So this method, confusingly, will not return true for a deduced `auto`, unless the deduced type is itself an undeduced `auto` (which I'm not sure can happen).  I think it at least needs a different name; `isUndeducedAutoType()` would be okay if the latter case is not possible.  But it might be better if we can just define away the need for the method entirely.


================
Comment at: lib/Sema/SemaType.cpp:7441
+      // the initializing expression type during the type deduction.
+      (T->isAutoType() && IsPointee) || (IsAutoPointee) ||
       // OpenCL spec v2.0 s6.9.b:
----------------
Anastasia wrote:
> mantognini wrote:
> > mantognini wrote:
> > > Shouldn't the parentheses around `IsAutoPointee` be removed for style consistency?
> > With the `if` statement introduced above, `IsAutoPointee` can be true only in C++ mode. Could it be an issue to not guard `(T->isAutoType() && IsPointee)` for non-C++ mode? (I guess not, but I couldn't convince myself.)
> I think `TreeTransforms` will only be used in C++ mode. But also `isAutoType` should only be true in C++ mode. So I think we should be fine.
I don't think `TreeTransform` is expected to be C++-only, but I agree that `isAutoType` is good enough.


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

https://reviews.llvm.org/D65744