[PATCH] D31397: [Bug 25404] Fix crash on typedef in OpenCL 2.0

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: lib/Sema/SemaDecl.cpp:2157
   if (getDiagnostics().getSuppressSystemWarnings() &&
-      (Context.getSourceManager().isInSystemHeader(Old->getLocation()) ||
+      // Some standard types are defined implicitly in Clang (e.g. OpenCL).
+      (Old->isImplicit() || New->isImplicit() ||
----------------
bader wrote:
> Anastasia wrote:
> > Anastasia wrote:
> > > ahatanak wrote:
> > > > Is it necessary to check whether New is implicit? I was just wondering when or how an implicit definition would redefine a typedef.
> > > I had a thought on it too, and I am not sure actually. This can happen if we implicitly define something from the standard headers. But I believe the Sema initialization should always happen before parsing the standard header or even loading them from the PCH. So I guess this shouldn't ever happen really? Perhaps, I should just remove this?
> > Actually in case of implicit typedefs we don't seem to follow this program path at all. So I am removing this.
> So something like this will also work?
> ```
> typedef float float4 __attribute((ext_vector_type(4)));
> typedef float4 atomic_int;
> ```
> 
An error will be given for this because the definitions are conflicting (incompatible).


================
Comment at: test/SemaOpenCL/types.cl:6
+// Check redefinition of standard types
+typedef atomic_int atomic_flag;
----------------
bader wrote:
> Can we check that -Wtypedef-redefinition will emit a warning for this expression?
> This typedef seems to be unnecessary since clang implicitly defines atomic_flag for OpenCL. User is not supposed to re-define it, so warning would be helpful here.
A warning is given only with -Wsystem-headers in this case. I don't modify this change here. But I can add a test if necessary. Verify wouldn't work here though because a note about the old location of the typedef will be given without source information. But I can add a FileCheck test.


https://reviews.llvm.org/D31397