<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><br></div><div>Generally, this patch looks really good. Thanks for the thorough set of test cases!</div><div><br></div><div><br></div><div>+void PragmaOptimizeHandler::HandlePragma(Preprocessor &PP, <br></div><div>
+ PragmaIntroducerKind Introducer,</div><div>+ Token &FirstToken) {</div><div>+ bool isOn = false;</div><div><br></div><div>Style: Please start variable names with an uppercase letter. Also, I'd prefer for this variable to be moved down next to where it's initialized.</div>
<div><br></div><div><br></div><div>+ PP.Lex(Tok);<br></div><div><br></div><div>Should we perform macro expansion on this token or not?</div><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">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>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><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>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">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>