<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><br></div><div><br></div><div><div>On Feb 2, 2014, at 23:16 , Daniel Fahlgren <<a href="mailto:daniel@fahlgren.se">daniel@fahlgren.se</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">Hi, Jordan.<br><br>On Thu, 2014-01-30 at 09:47 -0800, Jordan Rose wrote:<br><blockquote type="cite">Hi, Daniel. Can you split these two checks up into separate patches? I<br>know they're pretty similar and have the same basic idea, but we like<br>to have commits be limited to one new feature at a time. (Combining<br>bitwise and logical operators into one is fine, though.) Sorry it's a<br>bit more work for you to tease them apart.<br></blockquote><br>No problem. I have fixed your comments with the two attached patches.<br>if_stmt adds support to test if both branches are identical and should<br>be applied first. bin_op adds support to find identical expressions when<br>using binary operators.<br><br><blockquote type="cite">+ // Split operators as long as we still have operators to split on. We will<br>+ // get called for every binary operator in an expression so there is no need<br>+ // to check every one against each other here, just the right most one with<br>+ // the others.<br><br>Clever! Too bad it comes out to an N^2 algorithm, but N is likely<br>pretty small most of the time, and the nature of isIdenticalStmt<br></blockquote><br>Thanks. I thought about a better way to fix this when I implemented it,<br>but couldn't find a good one. First, somehow, sorting the expressions<br>and check only adjacent ones will probably be slower in practice due to<br>the overhead of sorting.<br><br><blockquote type="cite">Some missing test cases:<br><br>Should not warn: x == 4 && y == 5 || x == 4 && y == 6<br><br>Should warn: x ? "abc" : "abc"<br><br>Test cases with while, do, for, if, and return within the branches of<br>an if. If you add the code, we need to test them! We could also not<br>worry about this for now and just say the checker isn't powerful enough<br>to handle them.<br></blockquote><br>Test cases added.<br><br><blockquote type="cite">Why would that be useful? Because I'm a bit concerned about performance<br>for the if-branch-check. Can you try running this over a large-ish<br>project and let me know the before and after times of analyzing with<br>this checker on? If it's a significant difference we should put the<br>if-check under a separate checker name.<br></blockquote><br>I ran some tests with php 5.5.8 and openssl 1.0.1f. The tests were done<br>on my laptop with a ssd and 8GB of ram, so the numbers should be cpu<br>bound. Between each run I completely removed the directory and<br>reconfigured to make sure conditions were as similar as possible. The<br>checker was modified t only check the branches of an if statement, not<br>the binary operators.<br><br>Adding the check when running scan-build on php increased the time by<br>0.2%. On openssl it actually decreased by 0.15% instead. I think the<br>background tasks of my otherwise idle desktop has more influence on the<br>numbers than the test.<br><br>php 5.5.8 base run:<br><br>real 53m44.366s<br>user 51m31.245s<br>sys 1m11.288s<br><br>php 5.5.8 with checking if stmts:<br><br>real 53m51.311s<br>user 51m35.985s<br>sys 1m12.949s<br><br>openssl 1.0.1f base run:<br><br>real 11m40.819s<br>user 10m39.124s<br>sys 0m49.091s<br><br>openssl 1.0.1f with checking if stmts:<br><br>real 11m39.783s<br>user 10m38.536s<br>sys 0m48.879s<br><br>Best regards,<br>Daniel Fahlgren<br></blockquote><br></div><div>Thanks for running the timing checks. I agree that this seems pretty benign.</div><div><br></div><div>I thought of another controversial test case:</div><div><br></div><div>if (foo()) {</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>doSomething();</div><div>} else if (bar()) {</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>/* do nothing */</div><div>} else if (baz()) {</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>/* do nothing */</div><div>} else {</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>error();</div><div>}</div><div><br></div><div>It'd be pretty easy to just not check empty compound-statements. What do you think?</div><div><br></div><div>More test cases for the logical op checker:</div><div><br></div><div>x == 4 && y == 5 || x == 4</div><div>x == 4 || y == 5 && x == 4</div><div>x == 4 || x == 4 && y == 5</div><div><br></div><div>All of these would be fair to warn on, but we can probably get away with not for the first pass. If we <i>already</i> support one, though, should we add it as a regression test?</div><div><br></div><div>Finally, the typo "comparision" is still in the patch. :-)</div><div><br></div><div>Jordan</div><br></body></html>