<div dir="ltr">Use S.DiagRuntimeBehavior instead of S.Diag. Â The change to DiagRuntimeBehavior helps reduce the false positive rate on this warnings.<div><br></div><div>Move test/Sema/bool-compare.cpp to test/SemaCXX/bool-compare.cpp. Â Add tests when comparing against the Boolean constants true and false.<br>


<div class="gmail_extra"><br></div><div class="gmail_extra">For both tests, remove "-analyze" from the run lines. Â Also, it is not necessary to have so many variables. Â Having just 2-3 of each type would have been enough.<br>

<br><div class="gmail_extra">+ Â  Â  Â  Â // The constant value rests between values that OtherT can represent</div><div class="gmail_extra">+ Â  Â  Â  Â // after</div><div class="gmail_extra">+ Â  Â  Â  Â // conversion. Â Relational comparison still works, but equality</div>

<div class="gmail_extra">+ Â  Â  Â  Â // comparisons will be tautological.</div><div class="gmail_extra">Reflow the comments here.</div><div class="gmail_extra"><br></div><div class="gmail_extra">+ Â  Â if (Value.isNonNegative()) {<br>

</div><div class="gmail_extra">Check signedness before checking if non-negative. Â (Value.isUnsigned() || Value.isNonNegative())</div><div class="gmail_extra"><br></div><div class="gmail_extra">+ Â  Â  Â CmpRes = TruthTable.BO_LT_OP[(int)RhsConstant][(int)constVal];<br>

</div><div class="gmail_extra">Are these int casts necessary?</div><div class="gmail_extra"><br></div><div class="gmail_extra"><br></div><div class="gmail_quote">On Thu, Feb 20, 2014 at 2:03 AM, Per Viberg <span dir="ltr"><<a href="mailto:Per.Viberg@evidente.se" target="_blank">Per.Viberg@evidente.se</a>></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">




<div>
<div style="direction:ltr;font-size:10pt;font-family:Tahoma"><font>Hi,<br>
<br>
thanks for the input Richard, and <font>valid</font> points. <br>
<br>
I've corrected accordingly in the attached patch<font> </font>(see answered com<font>ments
</font>below). I saw that diagnos<font><font>tic</font> is recently changed from S.Diag t<font>o</font></font><font> S.DiagRuntimeBehavior</font><font>. That
<font>prevents this check from <font>warning inside templates</font></font><font> (see the very last test case in bool-c<font>o</font>mpare.cpp).
<font>I changed <font>it back to S.Diag<font> <font>
in this patch. Correct or not?.</font></font></font></font></font></font></font><font><font><font><font><font><br>
</font></font></font></font></font><font face="Tahoma"><font><br>
</font></font><font face="Tahoma"><font><font>I tried reusing the existing logic first, but it
<font>became very cryptic and hard to read</font><font><font><font><font>.
<font><font><font><font><font><font>I admit, it looks a b<font>it<font>
<font>bloated</font></font> to have <font>both logic and a truth table<font>. On the other hand insert<font>ing even more functionality
<font>in the already quite complex if/else someho<font>w didn't feel like the way to go.</font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font>
<font>I didn't start <font>to refactor existing functionality<font> because I didn't want to start off a discussion
<font>on whi<font>ch structure is the <font>best<font> (<font>I
<font>lean towards truth <font>tables<font> in m<font>ore complex logic</font></font>) so I j<font>ust added mine. It would be possible to unite them into one trutht<font>able</font>,
<font>altho<font>ugh that will make them more difficul<font>t to extend/modify later on.
</font></font></font></font></font></font></font></font></font></font></font></font></font></font><br>
<br>
<font face="Tahoma"><font><div><font>></font>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?
<br>
<br>
</div><font>--------------</font></font></font><br>
<br>
<div dir="ltr"><div>
<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><div>----: Now corrected, see the new patch.<br>
<br>
</div><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>
-----: Also corrected in this patch.<div><br>
<div><br>
<div style="font-size:13px;font-family:Tahoma">
<p class="MsoNormal"><span style="font-size:8pt;font-family:Arial,sans-serif;color:gray" lang="EN-US">.......................................................................................................................</span><span style="font-size:8pt;font-family:Arial,sans-serif" lang="EN-US"><br>



Per Viberg </span><span style="font-size:8pt;font-family:Arial,sans-serif;color:gray" lang="EN-US">Senior Engineer</span><span style="font-size:8.5pt;font-family:Arial,sans-serif;color:gray" lang="EN-US"><br>

Evidente ES East</span><span style="font-size:8pt;font-family:Arial,sans-serif;color:gray" lang="EN-US"> AB  Warfvinges väg 34  SE-112 51 Stockholm  Sweden
</span><span style="font-size:10pt;font-family:Tahoma,sans-serif" lang="EN-US"></span></p>
<p class="MsoNormal"><span style="font-size:8pt;font-family:Arial,sans-serif;color:gray" lang="EN-GB">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:     <a href="mailto:Per.Viberg@evidente.se" target="_blank"><font color="#0000ff">Per.Viberg@evidente.se</font></a>
</span><span style="font-size:8pt;font-family:Arial,sans-serif" lang="EN-GB"><br>
<br>
<a href="http://www.evidente.se" target="_blank"><font color="#0000ff">www.evidente.se</font></a></span></p>
<p class="MsoNormal"><span style="font-size:6pt;font-family:Arial,sans-serif" lang="EN-GB">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></p>
</div>
</div>
</div><div style="font-size:16px;font-family:'Times New Roman'">
<hr>
<div style="direction:ltr"><font color="#000000" face="Tahoma"><b>FrÃ¥n:</b> Richard Trieu [<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>]<br>
<b>Skickat:</b> den 5 februari 2014 01:44<br>
<b>Till:</b> Jordan Rose<br>
<b>Cc:</b> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a>; Per Viberg; Anders Rönnholm<div><div><br>
<b>Ämne:</b> Re: [PATCH] new check checking comparing of boolean expressions and literals<br>
</div></div></font><br>
</div><div><div>
<div></div>
<div>
<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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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><font color="#888888">
<div><br>
</div>
<div>Jordan</div>
</font></span>
<div>
<div>
<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 style="font-size:8pt;font-family:Arial,sans-serif;color:gray" lang="EN-US">.......................................................................................................................</span><span style="font-size:8pt;font-family:Arial,sans-serif" lang="EN-US"><br>



Per Viberg<span> </span></span><span style="font-size:8pt;font-family:Arial,sans-serif;color:gray" lang="EN-US">Senior Engineer</span><span style="font-size:8.5pt;font-family:Arial,sans-serif;color:gray" lang="EN-US"><br>



Evidente ES East</span><span style="font-size:8pt;font-family:Arial,sans-serif;color:gray" lang="EN-US"><span> </span>AB  Warfvinges väg 34  SE-112 51 Stockholm  Sweden</span><span style="font-size:10pt;font-family:Tahoma,sans-serif" lang="EN-US"></span></div>



<div style="margin-top:0px;margin-bottom:0px"><span style="font-size:8pt;font-family:Arial,sans-serif;color:gray" lang="EN-GB">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 style="font-size:8pt;font-family:Arial,sans-serif" lang="EN-GB"><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 style="font-size:6pt;font-family:Arial,sans-serif" lang="EN-GB">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 style="font-size:8pt;font-family:Arial,sans-serif;color:gray" lang="EN-US">.......................................................................................................................</span><span style="font-size:8pt;font-family:Arial,sans-serif" lang="EN-US"><br>



Per Viberg<span> </span></span><span style="font-size:8pt;font-family:Arial,sans-serif;color:gray" lang="EN-US">Senior Engineer</span><span style="font-size:8.5pt;font-family:Arial,sans-serif;color:gray" lang="EN-US"><br>



Evidente ES East</span><span style="font-size:8pt;font-family:Arial,sans-serif;color:gray" lang="EN-US"><span> </span>AB  Warfvinges väg 34  SE-112 51 Stockholm  Sweden</span><span style="font-size:10pt;font-family:Tahoma,sans-serif" lang="EN-US"></span></div>



<div style="margin-top:0px;margin-bottom:0px"><span style="font-size:8pt;font-family:Arial,sans-serif;color:gray" lang="EN-GB">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 style="font-size:8pt;font-family:Arial,sans-serif" lang="EN-GB"><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 style="font-size:6pt;font-family:Arial,sans-serif" lang="EN-GB">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>
</div>
</div></div></div>
</div>
</div>

</blockquote></div><br></div></div></div>