[PATCH] new check checking comparing of boolean expressions and literals
Jordan Rose
jordan_rose at apple.com
Mon Feb 3 09:51:16 PST 2014
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.
Jordan
On Jan 29, 2014, at 23:23 , Per Viberg <Per.Viberg at evidente.se> wrote:
>
> ping
>
> .......................................................................................................................
> 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
>
> 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: Per Viberg
> Skickat: den 20 december 2013 14:47
> Till: cfe-commits at cs.uiuc.edu
> Cc: Richard Trieu; Jordan Rose; Anders Rönnholm
> Ämne: SV: [PATCH] new check checking comparing of boolean expressions and literals
>
>
> Hi,
>
> I agree with comments below 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
>
> 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> 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> 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
> >
> > 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
> > 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/20140203/0f910a75/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: relationalCompare_rev4_4.diff
Type: application/octet-stream
Size: 28547 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140203/0f910a75/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140203/0f910a75/attachment-0001.html>
More information about the cfe-commits
mailing list