SV: [PATCH] new check checking comparing of boolean expressions and literals

[Per Viberg]

Hi,

I agree with comments from Richard Trieu.

I found an existing check, DiagnoseOutOfRangeComparison in SemaChecking.cpp that was checking for always true/false expressions by range checking variables.

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:

C++:
bool >  1: always false
bool <= 1: always true
bool <  0: always false
bool >= 0: always true

0 <= bool: always true
0 >  bool: always false
1 <  bool: always false
1 >= bool: always true

C:
boolean expression >  1 or positive values: always false
boolean expression <= 1 or positive values: always true
boolean expression <  0 or negative values: always false
boolean expression >= 0 or negative values: always true

0 or negative values <= boolean expression: always true
0 or negative values >  boolean expression: always false
1 or positive values <  boolean expression: always false
1 or positive values >= boolean expression: always true

patch is attached.

Best regards
/Per

.......................................................................................................................
Per Viberg Senior Engineer
Evidente ES East AB  Warfvinges väg 34  SE-112 51 Stockholm  Sweden
Phone:    +46 (0)8 402 79 00
Mobile:    +46 (0)70 912 42 52
E-mail:     Per.Viberg at evidente.se<mailto:Per.Viberg at evidente.se>

www.evidente.se<http://www.evidente.se>
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.
________________________________
Från: Richard Trieu [rtrieu at google.com]
Skickat: den 12 november 2013 22:57
Till: Jordan Rose
Cc: Per Viberg; cfe-commits at cs.uiuc.edu
Ämne: Re: [PATCH] new check checking comparing of boolean expressions and literals

This warning should not be in -Wtautological-compare since not all cases always evaluate to the same value every time.  For instance,

bool test(bool x) {
  return x > 0;
}

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.

On Tue, Nov 12, 2013 at 9:13 AM, Jordan Rose <jordan_rose at apple.com<mailto:jordan_rose at apple.com>> wrote:
Oops, two pings and still managed to miss this. Richard, can you take a look as well?

Some comments from me:

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

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

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

+  if (z ==(j<y))  {}   //
+  if (z<k>y)      {}        //
+  if (z > (l<y))  {}    //
+  if((z<y)>(n<y)) {}   //
+  if((z<y)==(o<y)){}  //
+  if((z<y)!=(p<y)){}  //
+  if((y==z)<(z==x)){}  //
+  if(((z==x)<(y==z))!=(q<y)){}//

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

- Wow, how did we miss UO_LNot as known-boolean?
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.

Jordan


On Oct 25, 2013, at 8:27 , Per Viberg <Per.Viberg at evidente.se<mailto:Per.Viberg at evidente.se>> wrote:

> Hi,
>
> Haven't received any comments, thus resending a patch from last week. Is anyone reviewing it?. it would be highly appreciated.
>
> Best regards,
> Per
>
>
>
> .......................................................................................................................
>
> Hello,
>
> The patch adds a check that warns for relational comparison of boolean expressions with literals 1 and 0.
> example:
>
> bool x;
> int i,e,y;
>
> if(x > 1){}
> if(!i < 0){}
> if(1 > (e<y)){}
>
> generates warning:
> "relational comparison of boolean expression against literal value '1' or '0' "
>
> see test files bool-compare.c and bool-compare.cpp for more details.
>
>
> Best regards,
> Per
>
> .......................................................................................................................
> Per Viberg Senior Engineer
> Evidente ES East AB  Warfvinges väg 34  SE-112 51 Stockholm  Sweden
> Phone:    +46 (0)8 402 79 00
> Mobile:    +46 (0)70 912 42 52
> E-mail:     Per.Viberg at evidente.se<mailto:Per.Viberg at evidente.se>
>
> www.evidente.se<http://www.evidente.se>
> 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.

> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131220/4506d6b6/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: relationalCompare_rev4_4.diff
Type: text/x-patch
Size: 28547 bytes
Desc: relationalCompare_rev4_4.diff
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131220/4506d6b6/attachment.bin>