<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>