<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Jan 23, 2012, at 6:40 AM, Dmitri Gribenko wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>On Fri, Jan 20, 2012 at 7:18 AM, Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com">gribozavr@gmail.com</a>> wrote:<br><blockquote type="cite">On Thu, Jan 19, 2012 at 1:11 AM, Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com">gribozavr@gmail.com</a>> wrote:<br></blockquote><blockquote type="cite"><blockquote type="cite">Attached is a patch that implements enhancement proposed in PR11329.<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">As suggested by Argyrios, this patch implements:<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">* a stack of "CompoundScopeInfo"s, helper functions to push/pop them,<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">ActOn{Start,Finish}OfCompoundStatement callbacks;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">* a check if the warning is actually enabled before doing costly checks.<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">Doing this uncovered a bug in TreeTransfom, Sema::ActOnBlockError was<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">not called in all cases. This is also fixed.<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">I've ran a chromium build again and found 3 false positives (-1<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">because one previously affected file now builds with -Wno-empty-body).<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">I accidentally sent an old version with an error. Here's a correct version.<br></blockquote><br>*ping*<br><br>Please review. Patch rebased to latest trunk and split in two.<br></div></blockquote><div><br></div><div>Sorry for the delay, committed the TreeTransform patch in r148915.</div><div><br></div><div>Looks good! Some nitpicks:</div><div><br></div><div>-Why did you remove "if-empty-body.cpp" ? Did you include all the test cases in warn-empty-body.cpp ?</div><div><br></div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">+/// \brief Contains information about the compound statement currently being</font></div><div><font class="Apple-style-span" color="#000000">+/// parsed.</font></div><div><font class="Apple-style-span" color="#000000">+class CompoundScopeInfo {</font></div><div><font class="Apple-style-span" color="#000000">+public:</font></div><div><font class="Apple-style-span" color="#000000">+ CompoundScopeInfo()</font></div><div><font class="Apple-style-span" color="#000000">+ : HasEmptyLoopBodies(false) { }</font></div><div><font class="Apple-style-span" color="#000000">+</font></div><div><font class="Apple-style-span" color="#000000">+ /// \brief Whether this compound stamement contains `for' or `while' loops</font></div><div><font class="Apple-style-span" color="#000000">+ /// with empty bodies.</font></div><div><font class="Apple-style-span" color="#000000">+ bool HasEmptyLoopBodies;</font></div><div><font class="Apple-style-span" color="#000000">+</font></div><div><font class="Apple-style-span" color="#000000">+ void setHasEmptyLoopBodies() {</font></div><div><font class="Apple-style-span" color="#000000">+ HasEmptyLoopBodies = true;</font></div><div><font class="Apple-style-span" color="#000000">+ }</font></div><div><font class="Apple-style-span" color="#000000">+};</font></div><div><font class="Apple-style-span" color="#000000">+</font></div></blockquote><div><div><br></div></div><div>You have a public field and a setter for it. Did you mean to actually make the field private so you cannot "unset" it ?</div><div><br></div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">+ SmallVector<CompoundScopeInfo *, 4> CompoundScopes;</font></div></blockquote><div><div><br></div></div><div>Why don't you push CompoundScopes as value objects, to avoid malloc traffic ?</div><div>to be more specific:</div><div><br></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 12px/normal Helvetica; ">SmallVector<CompoundScopeInfo, 4> CompoundScopes;</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 12px/normal Helvetica; ">CompoundScopeInfo &getCurCompoundScope() const;</div></div></div><div><br></div><div><br></div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">- </font></div><div><font class="Apple-style-span" color="#000000">+</font></div><div><font class="Apple-style-span" color="#000000">+ ActOnStartOfCompoundStmt();</font></div><div><font class="Apple-style-span" color="#000000"> StmtResult Body = ActOnCompoundStmt(Loc, Loc, move_arg(Statements),</font></div><div><font class="Apple-style-span" color="#000000"> /*isStmtExpr=*/false);</font></div><div><font class="Apple-style-span" color="#000000">+ ActOnFinishOfCompoundStmt();</font></div></blockquote><div><div><br></div></div><div>How about using a RAII object to make this less error prone, say something like a CompoundScopeRAII class, that will do the calls in the constructor/destructor.</div><div>This would be particularly useful inside TreeTransform<Derived>::TransformCompoundStmt.</div><div><br></div><div>-Argyrios</div></div></div><div><br></div><br><blockquote type="cite"><div><br>Dmitri<br><br>-- <br>main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if<br>(j){printf("%d\n",i);}}} /*Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com">gribozavr@gmail.com</a>>*/<br><span><tree-transform-act-on-block-error.patch></span><span><generalize-warn-empty-body-v9.patch></span>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits<br></div></blockquote></div><br></body></html>