[PATCH] new check checking comparing of boolean expressions and literals
Richard Trieu
rtrieu at google.com
Mon Mar 3 12:03:36 PST 2014
Use S.DiagRuntimeBehavior instead of S.Diag. The change to
DiagRuntimeBehavior helps reduce the false positive rate on this warnings.
Move test/Sema/bool-compare.cpp to test/SemaCXX/bool-compare.cpp. Add
tests when comparing against the Boolean constants true and false.
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.
+ // The constant value rests between values that OtherT can
represent
+ // after
+ // conversion. Relational comparison still works, but equality
+ // comparisons will be tautological.
Reflow the comments here.
+ if (Value.isNonNegative()) {
Check signedness before checking if non-negative. (Value.isUnsigned() ||
Value.isNonNegative())
+ CmpRes = TruthTable.BO_LT_OP[(int)RhsConstant][(int)constVal];
Are these int casts necessary?
On Thu, Feb 20, 2014 at 2:03 AM, Per Viberg <Per.Viberg at evidente.se> wrote:
> Hi,
>
> thanks for the input Richard, and valid points.
>
> I've corrected accordingly in the attached patch (see answered comments below).
> I saw that diagnostic is recently changed from S.Diag toS.DiagRuntimeBehavior.
> That prevents this check from warning inside templates (see the very last
> test case in bool-compare.cpp). I changed it back to S.Diag in this
> patch. Correct or not?.
>
> I tried reusing the existing logic first, but it became very cryptic and
> hard to read. I admit, it looks a bit bloated to have both logic and a
> truth table. On the other hand inserting even more functionality in the
> already quite complex if/else somehow didn't feel like the way to go. I
> didn't start to refactor existing functionality because I didn't want to
> start off a discussion on which structure is the best (I lean towards
> truth tables in more complex logic) so I just added mine. It would be
> possible to unite them into one truthtable, although that will make them
> more difficult to extend/modify later on.
>
> >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.
> ----: Now corrected, see the new patch.
>
> >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"
> -----: Also corrected in this patch.
>
>
>
> .......................................................................................................................
> 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 5 februari 2014 01:44
> *Till:* Jordan Rose
> *Cc:* cfe-commits at cs.uiuc.edu; Per Viberg; Anders Rönnholm
>
> *Ämne:* Re: [PATCH] new check checking comparing of boolean expressions
> and literals
>
> 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/20140303/2e5a9260/attachment.html>
More information about the cfe-commits
mailing list