[PATCH] new check checking comparing of boolean expressions and literals
Richard Trieu
rtrieu at google.com
Wed Mar 26 16:24:31 PDT 2014
Accidentally replied instead of replied all last time. Same response as
last time, but also including the mailing list this time:
Just a bunch of nit-picky stuff, then you should be ready to commit.
+ // Other isKnownToHaveBooleanValue
+ enum CompareBoolWithConstantResult { AFals, Unkwn, ATrue };
Put Unkwn last here.
+ enum constantValue { LT_Zero, Zero, One, GT_One, sizeOfConstVal };
Capitalize to ConstantValue, and SizeOfConstVal.
+ enum constantside { lhs, rhs, nofsides };
+ "comparison of %select{constant %0|true|false}4 with "
+ "%select{expression of type %2|boolean expression}1 is always "
+ "%select{false|true}3">, InGroup<TautologicalOutOfRangeCompare>;
Reorder the argument numbers, then change the diagnostic call to match. I
suggest:
+ "comparison of %select{constant %0|true|false}1 with "
+ "%select{expression of type %2|boolean expression}3 is always "
+ "%select{false|true}4">, InGroup<TautologicalOutOfRangeCompare>;
After that, feel free to commit.
On Fri, Mar 7, 2014 at 9:17 AM, Per Viberg <Per.Viberg at evidente.se> wrote:
> Hi Richard,
>
> I've addressed the issues you mentioned. Also added test cases against
> Boolean constants true/false as suggested.
>
>
>
> .......................................................................................................................
> 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 3 mars 2014 21:03
> *Till:* Per Viberg
> *Cc:* Jordan Rose; cfe-commits at cs.uiuc.edu; Anders Rönnholm; Daniel
> Marjamäki
>
> *Ämne:* Re: [PATCH] new check checking comparing of boolean expressions
> and literals
>
> 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
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>
>
On Fri, Mar 7, 2014 at 9:17 AM, Per Viberg <Per.Viberg at evidente.se> wrote:
> Hi Richard,
>
> I've addressed the issues you mentioned. Also added test cases against
> Boolean constants true/false as suggested.
>
>
>
> .......................................................................................................................
> 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 3 mars 2014 21:03
> *Till:* Per Viberg
> *Cc:* Jordan Rose; cfe-commits at cs.uiuc.edu; Anders Rönnholm; Daniel
> Marjamäki
>
> *Ämne:* Re: [PATCH] new check checking comparing of boolean expressions
> and literals
>
> 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/20140326/f523c8ed/attachment.html>
More information about the cfe-commits
mailing list