[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:
> > 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.
> Yes, `asm` is only explicitly restricted in OpenCL C++.  Not sure if it's safe to assume `asm` has been overlooked for OpenCL C though, it's not explicitly forbidden so people may be using it.
Do you have any contact with the OpenCL committee?  You might want to collect these issues and present them to them.  They may just not be aware that `asm` is just as much a feature in C as it is in C++, and it would be good to understand what the purpose of the `goto` restriction is.


================
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:
> > > > 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.
> Sorry if my previous comment was ambiguous: I meant here we now reject *any* thread storage class specifier for OpenCL C++, not just the `thread_local` spelling (my original patch was only rejecting `TSCS_thread_local`).
But you said we already diagnose this in Sema, so we don't need any code here.  If you want a special-case diagnostic, that's easy to do there; CUDA already does the same thing.


https://reviews.llvm.org/D46022