<div dir="ltr">Was there something wrong with reusing the existing logic instead of rewriting everything? It would be good to know what the problems with the existing checks are and possibly unite them later. I assume that CheckTrivialUnsignedComparison isn't properly checking 0 comparisons and nothing currently checks comparisons with the maximum/minimum value of a type? Is there anything else you are seeing?<div>
<br></div><div>With regards to the code, I don't like the padding trick you used to extend the array. Use multidimensional arrays inside LinkedConditions or make TruthTable an array of size 2. This will remove the need for the bit shifting later.</div>
<div><br></div><div>For the diagnostic message, it should not say "type 'bool'" if there is not bool in the language. I think the best we can do is say "boolean expression" and leave it at that.</div>
<div><br></div><div>The call to the diagnostic should be changed to:</div><div><div> S.Diag(E->getOperatorLoc(), diag::warn_out_of_range_compare)</div><div> << OS.str() << (OtherIsBooleanType && !OtherT->isBooleanType()) << OtherT << IsTrue</div>
<div> << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();</div></div><div><br></div><div>And warn_out_of_range_compare should be changed to:</div><div>"comparison of constant %0 with %select{boolean expression|expression of type %2}1 is always %select{false|true}3"<br>
</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Feb 3, 2014 at 9:51 AM, Jordan Rose <span dir="ltr"><<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>Richard, do you have any more comments for this? I have a few style notes I could give, but I think this is really better for you or someone else more in Sema to look at.</div>
<span class="HOEnZb"><font color="#888888"><div><br></div><div>Jordan</div></font></span><div><div class="h5"><div><br></div><br><div><div>On Jan 29, 2014, at 23:23 , Per Viberg <<a href="mailto:Per.Viberg@evidente.se" target="_blank">Per.Viberg@evidente.se</a>> wrote:</div>
<br><blockquote type="cite"><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div style="direction:ltr;font-family:Tahoma;font-size:10pt"><br>ping<br><div><br><div style="font-size:13px;font-family:Tahoma"><div style="margin-top:0px;margin-bottom:0px"><span lang="EN-US" style="font-size:8pt;font-family:Arial,sans-serif;color:gray">.......................................................................................................................</span><span lang="EN-US" style="font-size:8pt;font-family:Arial,sans-serif"><br>
Per Viberg<span> </span></span><span lang="EN-US" style="font-size:8pt;font-family:Arial,sans-serif;color:gray">Senior Engineer</span><span lang="EN-US" style="font-size:8.5pt;font-family:Arial,sans-serif;color:gray"><br>
Evidente ES East</span><span lang="EN-US" style="font-size:8pt;font-family:Arial,sans-serif;color:gray"><span> </span>AB Warfvinges väg 34 SE-112 51 Stockholm Sweden</span><span lang="EN-US" style="font-size:10pt;font-family:Tahoma,sans-serif"></span></div>
<div style="margin-top:0px;margin-bottom:0px"><span lang="EN-GB" style="font-size:8pt;font-family:Arial,sans-serif;color:gray">Phone: <a href="tel:%2B46%20%280%298%20402%2079%2000" value="+4684027900" target="_blank">+46 (0)8 402 79 00</a><br>
Mobile: <a href="tel:%2B46%20%280%2970%C2%A0912%2042%2052" value="+46709124252" target="_blank">+46 (0)70 912 42 52</a><br>E-mail: <span> </span><a href="mailto:Per.Viberg@evidente.se" target="_blank"><font color="#0000ff">Per.Viberg@evidente.se</font></a><span> </span></span><span lang="EN-GB" style="font-size:8pt;font-family:Arial,sans-serif"><br>
<br><a href="http://www.evidente.se/" target="_blank"><font color="#0000ff">www.evidente.se</font></a></span></div><div style="margin-top:0px;margin-bottom:0px"><span lang="EN-GB" style="font-size:6pt;font-family:Arial,sans-serif">This e-mail, which might contain confidential information, is addressed to the above stated person/company. If you are not the correct addressee, employee or in any other way the person concerned, please notify the sender immediately. At the same time, please delete this e-mail and destroy any prints. Thank You.</span></div>
</div></div><div style="font-family:'Times New Roman';font-size:16px"><hr><div style="direction:ltr"><font face="Tahoma"><b>Från:</b><span> </span>Per Viberg<br><b>Skickat:</b><span> </span>den 20 december 2013 14:47<br>
<b>Till:</b><span> </span><a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br><b>Cc:</b><span> </span>Richard Trieu; Jordan Rose; Anders Rönnholm<br><b>Ämne:</b><span> </span>SV: [PATCH] new check checking comparing of boolean expressions and literals<br>
</font><br></div><div></div><div><div style="direction:ltr;font-family:Tahoma;font-size:10pt"><br>Hi,<br><br>I agree with comments below from Richard Trieu.<span> </span><br><br>I found an existing check, DiagnoseOutOfRangeComparison in SemaChecking.cpp that was checking for always true/false expressions by range checking variables.<span> </span><br>
<br>It didn't find the special cases comparing boolean expressions with literals 1 and 0 though. Neither did it range check boolean expressions in C. I extended that check to detect always true/false cases of:<span> </span><br>
<font face="Courier New"><br>C++:<br>bool > 1: always false<br>bool <= 1: always true<br>bool < 0: always false<br>bool >= 0: always true<br><br>0 <= bool: always true<br>0 > bool: always false<br>1 < bool: always false<br>
1 >= bool: always true<br><br>C:<br>boolean expression > 1 or positive values: always false<br>boolean expression <= 1 or positive values: always true<br>boolean expression < 0 or negative values: always false<br>
boolean expression >= 0 or negative values: always true<br><br>0 or negative values <= boolean expression: always true<br>0 or negative values > boolean expression: always false<br>1 or positive values < boolean expression: always false<br>
1 or positive values >= boolean expression: always true<br></font><br>patch is attached.<br><br>Best regards<br>/Per<div><br><div style="font-size:13px;font-family:Tahoma"><div style="margin-top:0px;margin-bottom:0px">
<span lang="EN-US" style="font-size:8pt;font-family:Arial,sans-serif;color:gray">.......................................................................................................................</span><span lang="EN-US" style="font-size:8pt;font-family:Arial,sans-serif"><br>
Per Viberg<span> </span></span><span lang="EN-US" style="font-size:8pt;font-family:Arial,sans-serif;color:gray">Senior Engineer</span><span lang="EN-US" style="font-size:8.5pt;font-family:Arial,sans-serif;color:gray"><br>
Evidente ES East</span><span lang="EN-US" style="font-size:8pt;font-family:Arial,sans-serif;color:gray"><span> </span>AB Warfvinges väg 34 SE-112 51 Stockholm Sweden</span><span lang="EN-US" style="font-size:10pt;font-family:Tahoma,sans-serif"></span></div>
<div style="margin-top:0px;margin-bottom:0px"><span lang="EN-GB" style="font-size:8pt;font-family:Arial,sans-serif;color:gray">Phone: <a href="tel:%2B46%20%280%298%20402%2079%2000" value="+4684027900" target="_blank">+46 (0)8 402 79 00</a><br>
Mobile: <a href="tel:%2B46%20%280%2970%C2%A0912%2042%2052" value="+46709124252" target="_blank">+46 (0)70 912 42 52</a><br>E-mail: <span> </span><a href="mailto:Per.Viberg@evidente.se" target="_blank"><font color="#0000ff">Per.Viberg@evidente.se</font></a><span> </span></span><span lang="EN-GB" style="font-size:8pt;font-family:Arial,sans-serif"><br>
<br><a href="http://www.evidente.se/" target="_blank"><font color="#0000ff">www.evidente.se</font></a></span></div><div style="margin-top:0px;margin-bottom:0px"><span lang="EN-GB" style="font-size:6pt;font-family:Arial,sans-serif">This e-mail, which might contain confidential information, is addressed to the above stated person/company. If you are not the correct addressee, employee or in any other way the person concerned, please notify the sender immediately. At the same time, please delete this e-mail and destroy any prints. Thank You.</span></div>
</div></div><div style="font-family:'Times New Roman';font-size:16px"><hr><div style="direction:ltr"><font face="Tahoma"><b>Från:</b><span> </span>Richard Trieu [<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>]<br>
<b>Skickat:</b><span> </span>den 12 november 2013 22:57<br><b>Till:</b><span> </span>Jordan Rose<br><b>Cc:</b><span> </span>Per Viberg;<span> </span><a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<b>Ämne:</b><span> </span>Re: [PATCH] new check checking comparing of boolean expressions and literals<br></font><br></div><div></div><div><div dir="ltr">This warning should not be in -Wtautological-compare since not all cases always evaluate to the same value every time. For instance,<div>
<br></div><div>bool test(bool x) {</div><div> return x > 0;</div><div>}</div><div><br></div><div>This function can still return true or false. Check the other warnings in -Wtautological-compare which has their messages end with "always evaluates to " then some value. Otherwise, this warning basically boil down to users not writing code in the way we expected them to and then producing a vague warning. Possibly, split out the tautological parts to go into -Wtautological-compare, then add additional notes, maybe with fix-it hints, to help the user out.<div class="gmail_extra">
<br><div class="gmail_quote">On Tue, Nov 12, 2013 at 9:13 AM, Jordan Rose<span> </span><span dir="ltr"><<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>></span><span> </span>wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Oops, two pings and still managed to miss this. Richard, can you take a look as well?<br>
<br>Some comments from me:<br><br>- I would check for the IntegerLiteral first, because that’s a simple isa<> check, whereas isKnownToHaveBooleanValue has to look at the expression in more detail.<br><br>- We tend not to include empty else cases in LLVM code, even for commenting purposes. “else { return; }” is more okay, but in general we try to keep nesting and braces to a minimum.<br>
<br>- String values can be broken up in TableGen, just like in C. Please keep the indentation consistent and break up the line into multiple string literal chunks.<br><br>+ if (z ==(j<y)) {} //<br>+ if (z<k>y) {} //<br>
+ if (z > (l<y)) {} //<br>+ if((z<y)>(n<y)) {} //<br>+ if((z<y)==(o<y)){} //<br>+ if((z<y)!=(p<y)){} //<br>+ if((y==z)<(z==x)){} //<br>+ if(((z==x)<(y==z))!=(q<y)){}//<br>
<br>- Some funky indentation on this set of test comments. I would actually just drop all the empty comment lines, or add "no-warning” (which doesn’t do anything, but looks like expected-warning).<br><br>- Wow, how did we miss UO_LNot as known-boolean?<br>
</blockquote><div>I know I missed it since I usually do getType()->isBooleanType(), which worked great for C++ which has a boolean type, but not so well for C which does not. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span><font color="#888888"><br>Jordan<br></font></span><div><div><br><br>On Oct 25, 2013, at 8:27 , Per Viberg <<a href="mailto:Per.Viberg@evidente.se" target="_blank">Per.Viberg@evidente.se</a>> wrote:<br><br>> Hi,<br>
><br>> Haven't received any comments, thus resending a patch from last week. Is anyone reviewing it?. it would be highly appreciated.<br>><br>> Best regards,<br>> Per<br>><br>><br>><br>> .......................................................................................................................<br>
><br>> Hello,<br>><br>> The patch adds a check that warns for relational comparison of boolean expressions with literals 1 and 0.<br>> example:<br>><br>> bool x;<br>> int i,e,y;<br>><br>> if(x > 1){}<br>
> if(!i < 0){}<br>> if(1 > (e<y)){}<br>><br>> generates warning:<br>> "relational comparison of boolean expression against literal value '1' or '0' "<br>><br>> see test files bool-compare.c and bool-compare.cpp for more details.<br>
><br>><br>> Best regards,<br>> Per<br>><br>> .......................................................................................................................<br>> Per Viberg Senior Engineer<br>
> Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden<br>> Phone: <a href="tel:%2B46%20%280%298%20402%2079%2000" value="+4684027900" target="_blank">+46 (0)8 402 79 00</a><br>> Mobile: <a href="tel:%2B46%20%280%2970%20912%2042%2052" value="+46709124252" target="_blank">+46 (0)70 912 42 52</a><br>
> E-mail: <span> </span><a href="mailto:Per.Viberg@evidente.se" target="_blank">Per.Viberg@evidente.se</a><br>><br>><span> </span><a href="http://www.evidente.se/" target="_blank">www.evidente.se</a><br>> This e-mail, which might contain confidential information, is addressed to the above stated person/company. If you are not the correct addressee, employee or in any other way the person concerned, please notify the sender immediately. At the same time, please delete this e-mail and destroy any prints. Thank You.<br>
</div></div><br>> _______________________________________________<br>> cfe-commits mailing list<br>><span> </span><a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>><span> </span><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><br></blockquote></div><br></div></div></div></div></div></div></div></div></div></div></blockquote></div></div></div></div><br><div style="word-wrap:break-word"><div><blockquote type="cite"><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
</div></blockquote></div><br></div>
<br></blockquote></div><br></div>