[PATCH] new check checking comparing of boolean expressions and literals
Richard Trieu
rtrieu at google.com
Thu Apr 3 21:20:44 PDT 2014
I have committed this patch at r205608.
On Wed, Apr 2, 2014 at 10:16 AM, Anders Rönnholm <
Anders.Ronnholm at evidente.se> wrote:
> 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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140403/aeab2449/attachment.html>
More information about the cfe-commits
mailing list