<div dir="ltr">Hi Richard.<div>Thanks for the quick reply and for the review!</div><div><br></div><div>I am attaching a new patch which fixes the style issues and adds a specific test case for all the expected diagnostics coming from the handling of '#pragma clang optimize'.</div>
<div><br></div><div class="gmail_extra"><br><div class="gmail_quote">On 6 May 2014 22:26, 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>What should happen if a preprocessed header or a TU preamble ends with a "#pragma clang optimize off" directive active? For a module, I don't think it should have any effect, but in those other two cases I think we should reactivate the directive when loading the preamble or PCH.</div>
</div></blockquote><div><br></div><div>Yes, if we reach the end of a compilation unit and the state of the pragma is still "off" then nothing special happens. This is because users may want to disable optimizations in a whole file by just having '#pragma clang optimize off' at the top of the file, and they shouldn't need a matching "on" at the end.</div>
<div>However, you make a good point about PCH. I haven't looked at this in detail, but I suppose we would have to somehow serialize the state of OptimizeOffPragmaLocation in the precompiled header. This should be enough, because any function definition in the header (if any is present) would have been decorated with 'optnone' already, so at the point of serialization the effect of the pragma has already been applied and we just serialize the attributes as usual.</div>
<div><br></div><div>At the moment the patch does not include serialization in the PCH, but I will have a look at how it's done and implement that too.</div><div>Would you prefer to have just one patch with everything in it, or would you be OK with getting this in and then incrementally improve it with another patch?</div>
<div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">
<div>+ PP.Lex(Tok);<br></div><div><br></div><div>Should we perform macro expansion on this token or not?</div></div></blockquote><div><br></div><div>Apparently, by the time the token gets there it has already been expanded.</div>
<div>I have verified this in the additional test case, with:</div><div>#define OFF off</div><div>#define ON on</div><div>#pragma clang optimize OFF</div><div>#pragma clang optimize ON</div><div>It does not seem to produce any diagnostic, so the tokens are processed as 'off' and 'on'.</div>
<div><br></div><div>If you are OK with the updated patch, could you please commit it for me? (I don't have commit access yet)</div><div><br></div><div>Cheers,<br></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><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>
<br></div><div><br></div><div><div>+ if (II->isStr("on")) {</div>
<div>+ isOn = true;</div><div>+ PP.Lex(Tok);</div><div>+ }</div><div>+ else if (II->isStr("off"))</div><div>+ PP.Lex(Tok);</div><div>+ else {</div><div>+ PP.Diag(Tok.getLocation(), diag::err_pragma_optimize_malformed);</div>
<div>+ return;</div><div>+ }</div></div><div><br></div><div>Please sink the PP.Lex call below this if/else block. Also, you don't seem to have any test coverage for the 'malformed' case.</div><div><br></div>
<div><br></div><div><div>+ /// OptimizeOffPragmaLocation - it represents the last location of a "#pragma</div><div>+ /// clang optimize off" directive if such directive has not been closed by an</div><div>+ /// "on" yet. If optimizations are currently "on", this is set to an invalid</div>
<div>+ /// location.</div><div>+ SourceLocation OptimizeOffPragmaLocation;</div></div><div><br></div><div>Style: Please start this comment with "\brief" instead of the name of the entity being declared. (Likewise for the next three declarations.)</div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote"><div><div class="h5">On Tue, May 6, 2014 at 6:38 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"><div dir="ltr"><div>Hi,</div><div><br></div><div>Please review the attached patch for adding "pragma clang optimize on/off" to clang.</div>
<div>This pragma is implemented by decorating function definitions in the "off" regions with attribute 'optnone'.</div>
<div><br></div><div>The syntax and semantics have been discussed in here:</div><div><a href="http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-April/036561.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-April/036561.html</a><br>
</div><div>The final consensus was that the feature was worth having, and after a while I think we converged on the syntax, so I am now sending the patch for review.</div><div><br></div><div>Here is a very short summary of how the pragma works:</div>
<div><br></div><div> // The default status is "on".</div><div>#pragma clang optimize off</div><div> // Function definitions in here are decorated with attribute 'optnone' </div><div> // but only if it does not conflict with existing attributes.</div>
<div>#pragma clang optimize on</div><div> // Code compiled as normal.</div><div><br></div><div>The test provided with the patch checks a number of interesting cases, including template definitions and instantiations, macros, and conflicting attributes (e.g. always_inline).</div>
<div><br></div><div>I will prepare another patch for the documentation.</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>
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></blockquote></div><br></div>
</blockquote></div><br></div></div>