svenvh added inline comments.
================
Comment at: lib/Parse/ParseStmtAsm.cpp:696
+ return StmtError();
+ }
+
----------------
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.
================
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:
----------------
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`).
https://reviews.llvm.org/D46022
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
