<div dir="ltr">Hi Richard, and all.<div><br></div><div>Thank you very much for the review of this patch and for the useful comments.</div><div>I have made the final two changes that Richard requested, and I have committed revision 209510. My first actual commit! :-)</div>
<div><br></div><div>Cheers,</div><div> Dario Domizioli</div><div> SN Systems - Sony Computer Entertainment Group</div><div><br></div><div><br></div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">
On 23 May 2014 01:41, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div class="">On Fri, May 16, 2014 at 6:53 AM, Dario Domizioli <span dir="ltr"><<a href="mailto:dario.domizioli@gmail.com" target="_blank">dario.domizioli@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Hello again, everybody.<div><br><div>Here is an updated and rebased patch which addresses Sean's comments about the very ambiguous diagnostic message. I have now made it an error - after all, extra arguments mean that the pragma as written has an invalid syntax and the user intent cannot really be guessed.</div>
</div><div>I have also added a check that the pragma works with the _Pragma operator as well (so that you can #define it).</div><div><br></div><div>I think we're converging on a solid patch.</div><div>@Richard: could you tell me if you are satisfied with the serialization support as implemented?</div>
</div></blockquote><div><br></div></div><div><div>+ WriteOptimizePragmaOptions(SemaRef);<br></div></div><div><br></div><div>Please guard this by if (!WritingModule). I don't think it makes sense to inherit this state from a module (and I definitely don't want to worry about merging it if multiple modules disagree about the state). Since it might then be missing...</div>
<div><br></div><div><div>+ // Update the state of 'pragma clang optimize'. Use the same API as if we had</div><div>+ // encountered the pragma in the source.</div><div>+ bool IsOn = !OptimizeOffPragmaLocation.isValid();</div>
<div>+ SemaObj->ActOnPragmaOptimize(IsOn, OptimizeOffPragmaLocation);</div></div><div><br></div><div>... only call ActOnPragmaOptimize if the location is valid.</div><div><br></div><div>Otherwise, this LGTM.</div><div>
<div class="h5"><div>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>Thanks,</div><div><div>
Dario Domizioli</div><div> SN Systems - Sony Computer Entertainment Group</div><div><br></div><div><br></div><div><br></div><div><br></div><div><br></div><div><br></div></div></div><div><div><div class="gmail_extra">
<br><br><div class="gmail_quote">On 13 May 2014 20:50, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote"><div>On Mon, May 12, 2014 at 1:32 PM, Dario Domizioli <span dir="ltr"><<a href="mailto:dario.domizioli@gmail.com" target="_blank">dario.domizioli@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Hi Sean.<br><div><div class="gmail_extra">
<br><br><div class="gmail_quote"><div>On 12 May 2014 19:36, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>+#pragma clang optimize on top of spaghetti // expected-warning {{extra tokens at end of '#pragma clang optimize' - ignored}}<br>
<div></div></div><div>The diagnostic here is ambiguous. Was the pragma itself ignored or were the extra tokens ignored?</div><span><font color="#888888">
<div><br></div></font></span></div></blockquote><div><br></div></div><div>Hm... The whole pragma is ignored in that case. Some other pragmas behave the same - if they are not correctly formatted and they have additional argument they are ignored as a whole. Now that I think of it, I agree that this behaviour is confusing.<br>
I have maintained consistency with other pragmas (ahem... ok, maybe I have copied the check from another pragma handler :-) ), but I could easily change this specific case to an error. In fact, now I think it would make more sense.<br>
</div><div><br></div><div>As for the actual text being printed out, the problem is that I have leveraged an existing warning message. I could change the message, but it would suddenly become different in all the pragmas that make use of the message. If we do that, it would probably be better to make it a separate, documented change.<br>
</div></div></div></div></div></blockquote><div><br></div></div><div>Yeah, that definitely would be better suited to being its own change.</div><span><font color="#888888"><div><br></div><div>-- Sean Silva</div>
</font></span><div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div><div class="gmail_extra"><div class="gmail_quote"><div>
<br></div><div><div>Cheers,<br></div><div> Dario Domizioli<br></div><div> SN Systems - Sony Computer Entertainment Group<br></div><div><br></div><div> </div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div dir="ltr"><span></span>PS: I actually laughed out loud when I read "on top of spaghetti" :)</div></blockquote><div><br></div><div>I cannot claim the paternity of the phrase since I copied it from another pragma test. :-) But I found it funny too.<br>
<br><br><br><br><br><br><br></div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div class="gmail_extra">
<br><br><div class="gmail_quote">
<div><div>
On Thu, May 8, 2014 at 10:43 AM, Dario Domizioli <span dir="ltr"><<a href="mailto:dario.domizioli@gmail.com" target="_blank">dario.domizioli@gmail.com</a>></span> wrote:<br>
</div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div><div><div dir="ltr">Hello again!<div><br>
</div><div>I have now extended the patch with support for serialization of the state of the pragma (for PCHs). I have followed a similar pattern to the one used for the floating point pragmas and the diagnostic pragmas.</div>
<div>I have added a PCH test that verifies that if a "#pragma clang optimize off" is still active at the end of a PCH then a source file compiled including the PCH will behave as if the pragma was specified in the source (as it happens with normal headers). The test is modeled after the pragma diagnostics test.</div>
<div><br></div><div>I hope the new attached patch covers the issue that was raised by Richard... but I welcome any feedback. I might have missed something.</div><div><div><br></div><div>Cheers,</div><div> Dario Domizioli</div>
<div> SN Systems - Sony Computer Entertainment Group</div><div><br></div><div><br></div><div><br></div><div><br></div><div><br></div><div><br></div></div></div><div><div><div class="gmail_extra">
<br><br><div class="gmail_quote">On 7 May 2014 18:41, Dario Domizioli <span dir="ltr"><<a href="mailto:dario.domizioli@gmail.com" target="_blank">dario.domizioli@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra">Thanks Aaron!<br>
<br><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
Patch LGTM, modulo Richard's comments about serialization (I worried<br>
about the same thing, and my preference is for the patch to be<br>
all-inclusive when feasible, but Richard is welcome to weigh in). I<br>
have no strong opinions on whether it should be one diagnostic or two.<br></blockquote><div><br></div></div><div>I will wait for Richard's comments then; meanwhile I'll keep working on the serialization issue.</div>
<div>
<br></div><div>As for the diagnostics, I think that having two might make it easier to extend the "unexpected" case if the pragma gains new functionality in the future, while the "missing" case is unlikely to change... although the %select is quite powerful so I haven't got a strong preference either.</div>
<div>
<div><br></div><div>Cheers,</div><div> Dario Domizioli</div><div> SN Systems - Sony Computer Entertainment Group</div><div><br></div><div><br></div></div></div></div></div>
</blockquote></div><br></div>
</div></div><br></div></div><div>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></div></blockquote></div><br></div>
</blockquote></div></div><br></div></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>