<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;"><br><div><div>On Feb 4, 2014, at 13:05 , 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 Mon, 2014-02-03 at 18:37 -0800, Jordan Rose wrote:<br><blockquote type="cite">Thanks for running the timing checks. I agree that this seems pretty<br>benign.<br><br>I thought of another controversial test case:<br></blockquote><br><blockquote type="cite">if (foo()) {<br>doSomething();<br>} else if (bar()) {<br>/* do nothing */<br>} else if (baz()) {<br>/* do nothing */<br>} else {<br>error();<br>}<br></blockquote><br><blockquote type="cite">It'd be pretty easy to just not check empty compound-statements. What<br>do you think?<br></blockquote><br>I fail to understand the problem. Can you elaborate, please? If we<br>rearrange the code a bit we get<br><br> if (foo()) {<br> doSomething();<br> } else<br> if (bar()) {<br> /* do nothing */<br> } else<br> if (baz()) {<br> /* do nothing */<br> } else {<br> error();<br> } <br><br>with no two if-then-else-branches identical. The current code doesn't<br>(correctly?) warn about this.<br></blockquote><div><br></div><div>Hm, you're right; the current code doesn't handle this, and perhaps shouldn't (even in the non-empty case). How about this case, though?</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>/* no action needed */</div><div>} else {</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>/* FIXME: Implement this. */</div><div>}</div><div><br></div><div>This one <i>will</i> get flagged, correct? Should it?</div><div><br></div><div>(You could argue that this isn't great code, but since it's easy to special-case this I wanted to ask what you think.)</div><div><br></div><div><br></div><br><blockquote type="cite"><br>A future addition might be to detect the same logical expressions in<br>multiple nested if statements, like:<br><br> if (i == 1) {<br> doSomething1();<br> } else if (i == 2) {<br> doSomething2();<br> } else if (i == 2) {<br> doSomething3();<br> } else {<br> error();<br> }<br></blockquote><div><br></div><div>Yes, that would <i>definitely</i> be a nice check.</div><div><br></div><br><blockquote type="cite"><blockquote type="cite"><br>More test cases for the logical op checker:<br></blockquote><br><blockquote type="cite">x == 4 && y == 5 || x == 4<br>x == 4 || y == 5 && x == 4<br>x == 4 || x == 4 && y == 5<br><br>All of these would be fair to warn on, but we can probably get away<br>with not for the first pass. If we already support one, though, should<br>we add it as a regression test?<br></blockquote><br>Unfortunately we don't support that right now, but I agree it would be a<br>good addition.<br></blockquote><div><br></div><div>I would put them in anyway, then, even if there's no warning today.</div><div><br></div><div>Jordan</div></div></body></html>