[PATCH] D46022: [OpenCL] Restrict various keywords in OpenCL C++ mode

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: lib/Parse/ParseStmtAsm.cpp:696
+    return StmtError();
+  }
+
----------------
svenvh wrote:
> rjmccall wrote:
> > You might consider parsing the statement normally and then just diagnosing after the fact, maybe in Sema.  You'd have to add the check in a couple different places, though.
> Precisely the reason why I did it in the parser, otherwise we'd have to duplicate the check in multiple places.
The downside of this is that the parser recovery is probably very bad.  But since this is asm, it's not likely to matter too much.

...is this *also* really only an OpenCL C++ restriction?  Maybe we should read this one as a general restriction that happens to have been overlooked in the OpenCL C spec.


================
Comment at: lib/Sema/DeclSpec.cpp:621
+    // OpenCL C++ 1.0 s2.9: the thread_local storage qualifier is not
+    // supported.
+    case TSCS_thread_local:
----------------
svenvh wrote:
> rjmccall wrote:
> > svenvh wrote:
> > > rjmccall wrote:
> > > > Do you really care about the spelling of the specifier?  Presumably `__thread` (the GNU extension) and `_Thread_local` (the C11 feature) are unsupported; where are those diagnosed?
> > > Fair enough, I have updated the patch to just reject any thread qualifier here.
> > I meant it as a question, but I take it from this response that we don't actually diagnose these.
> > 
> > It might make more sense to diagnose this in `Sema::ActOnVariableDeclarator`, where we're already doing some checking about whether e.g. the target supports the feature.
> We do actually diagnose the others in `Sema::ActOnVariableDeclarator` ("thread-local storage is not supported for the current target").  But your question made me realize there is no real need to differentiate here for the OpenCL C++ case.
Okay.  So we can remove this code, then.


https://reviews.llvm.org/D46022