SV: [PATCH] new check checking comparing of boolean expressions and literals
Anders Rönnholm
Anders.Ronnholm at evidente.se
Wed Apr 2 10:16:04 PDT 2014
Made that last changes you wanted. We don't have commit rights so if you or someone else with the ability to commit can do that it would be much appreciated.
//Anders
.......................................................................................................................
Anders Rönnholm Senior Engineer
Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden
Mobile: +46 (0)70 912 42 54
E-mail: Anders.Ronnholm at evidente.se
www.evidente.se
________________________________________
Från: Richard Trieu [rtrieu at google.com]
Skickat: den 27 mars 2014 00:24
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
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<mailto: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<tel:%2B46%20%280%298%20402%2079%2000>
Mobile: +46 (0)70 912 42 52<tel:%2B46%20%280%2970%C2%A0912%2042%2052>
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<mailto:rtrieu at google.com>]
Skickat: den 3 mars 2014 21:03
Till: Per Viberg
Cc: Jordan Rose; cfe-commits at cs.uiuc.edu<mailto: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<mailto: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 to S.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<tel:%2B46%20%280%298%20402%2079%2000>
Mobile: +46 (0)70 912 42 52<tel:%2B46%20%280%2970%C2%A0912%2042%2052>
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<mailto:rtrieu at google.com>]
Skickat: den 5 februari 2014 01:44
Till: Jordan Rose
Cc: cfe-commits at cs.uiuc.edu<mailto: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<mailto: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<mailto: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<tel:%2B46%20%280%298%20402%2079%2000>
Mobile: +46 (0)70 912 42 52<tel:%2B46%20%280%2970%C2%A0912%2042%2052>
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: Per Viberg
Skickat: den 20 december 2013 14:47
Till: cfe-commits at cs.uiuc.edu<mailto: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<tel:%2B46%20%280%298%20402%2079%2000>
Mobile: +46 (0)70 912 42 52<tel:%2B46%20%280%2970%C2%A0912%2042%2052>
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<mailto:rtrieu at google.com>]
Skickat: den 12 november 2013 22:57
Till: Jordan Rose
Cc: Per Viberg; cfe-commits at cs.uiuc.edu<mailto: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<tel:%2B46%20%280%298%20402%2079%2000>
> Mobile: +46 (0)70 912 42 52<tel:%2B46%20%280%2970%20912%2042%2052>
> 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
On Fri, Mar 7, 2014 at 9:17 AM, Per Viberg <Per.Viberg at evidente.se<mailto: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<tel:%2B46%20%280%298%20402%2079%2000>
Mobile: +46 (0)70 912 42 52<tel:%2B46%20%280%2970%C2%A0912%2042%2052>
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<mailto:rtrieu at google.com>]
Skickat: den 3 mars 2014 21:03
Till: Per Viberg
Cc: Jordan Rose; cfe-commits at cs.uiuc.edu<mailto: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<mailto: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 to S.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<tel:%2B46%20%280%298%20402%2079%2000>
Mobile: +46 (0)70 912 42 52<tel:%2B46%20%280%2970%C2%A0912%2042%2052>
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<mailto:rtrieu at google.com>]
Skickat: den 5 februari 2014 01:44
Till: Jordan Rose
Cc: cfe-commits at cs.uiuc.edu<mailto: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<mailto: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<mailto: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<tel:%2B46%20%280%298%20402%2079%2000>
Mobile: +46 (0)70 912 42 52<tel:%2B46%20%280%2970%C2%A0912%2042%2052>
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: Per Viberg
Skickat: den 20 december 2013 14:47
Till: cfe-commits at cs.uiuc.edu<mailto: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<tel:%2B46%20%280%298%20402%2079%2000>
Mobile: +46 (0)70 912 42 52<tel:%2B46%20%280%2970%C2%A0912%2042%2052>
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<mailto:rtrieu at google.com>]
Skickat: den 12 november 2013 22:57
Till: Jordan Rose
Cc: Per Viberg; cfe-commits at cs.uiuc.edu<mailto: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<tel:%2B46%20%280%298%20402%2079%2000>
> Mobile: +46 (0)70 912 42 52<tel:%2B46%20%280%2970%20912%2042%2052>
> 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 --------------
A non-text attachment was scrubbed...
Name: relationalCompare_rev7.diff
Type: text/x-patch
Size: 31566 bytes
Desc: relationalCompare_rev7.diff
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140402/f62c21d2/attachment.bin>
More information about the cfe-commits
mailing list