[PATCH] new check checking comparing of boolean expressions and literals
Richard Trieu
rtrieu at google.com
Tue Feb 4 16:44:18 PST 2014
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?
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.
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.
The call to the diagnostic should be changed to:
S.Diag(E->getOperatorLoc(), diag::warn_out_of_range_compare)
<< OS.str() << (OtherIsBooleanType && !OtherT->isBooleanType()) <<
OtherT << IsTrue
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
And warn_out_of_range_compare should be changed to:
"comparison of constant %0 with %select{boolean expression|expression of
type %2}1 is always %select{false|true}3"
On Mon, Feb 3, 2014 at 9:51 AM, Jordan Rose <jordan_rose at apple.com> wrote:
> 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/20140204/d3805a44/attachment.html>
More information about the cfe-commits
mailing list