[PATCH] check for Incorrect logic in operator
Richard Trieu
rtrieu at google.com
Wed Mar 26 20:28:36 PDT 2014
So, it looks like one of your new warning in compareBoolWithInt is sort of
the same as Per's patch to extend -Wtautological-compare warning for better
bool handling. Keep the existing logical, just remove the
warning warn_bool_and_int_comparison and the compareBoolWithInt() function.
warn-compint.c should also be removed.
+ if(BuildOpts.Observer)
+ BuildOpts.Observer->compareBoolWithInt(B);
Add space after 'if'
TautologicalCompareOperator isn't very descriptive. Once the bool compares
are removed, I think TautologicalOverlapCompare would be a better name for
it.
On Fri, Mar 14, 2014 at 7:51 AM, Anders Rönnholm <
Anders.Ronnholm at evidente.se> wrote:
> Hi Richard,
>
> Modified the patch according to your comments.
>
> Except for Use the APSInt ++operator as i don't want to increment L1 or L2.
>
> //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 6 mars 2014 06:25
> Till: Anders Rönnholm
> Cc: Richard Smith; cfe-commits at cs.uiuc.edu
> Ämne: Re: [PATCH] check for Incorrect logic in operator
>
> + const IntegerLiteral *Literal1 =
> + dyn_cast<IntegerLiteral>(LHS->getRHS()->IgnoreParens());
> Note that this won't work on negative values.
>
> + Literal1->EvaluateAsInt(L1,*Context);
> + Literal2->EvaluateAsInt(L2,*Context);
> +
> + if (!L1 || !L2)
> + return TryResult();
> if (!Literal1->EvaluateAsInt(L1, *Context) || Literal2->EvaluateAsInt(L2,
> *Context))
> return TryResult();
>
> + const llvm::APSInt MinValue =
> + llvm::APSInt::getMinValue(L1.getBitWidth(), L1.isUnsigned());
> + const llvm::APSInt MaxValue =
> + llvm::APSInt::getMaxValue(L1.getBitWidth(), L1.isUnsigned());
> Move these into the array below.
>
> + llvm::APSInt I1 = llvm::APSInt(llvm::APInt(L1.getBitWidth(), 1),
> + L1.isUnsigned());
> Don't need this.
>
> + // Value between Value1 and Value2
> + ((L1 < L2) ? L1 : L2) + I1,
> Use the APSInt ++operator.
>
> + if(AlwaysTrue || AlwaysFalse) {
> + if(BuildOpts.Observer)
> Add space between the if and the parentheses.
>
> + void compareAlwaysTrue(const BinaryOperator* B, bool isAlwaysTrue) {
> BinaryOperator *B
>
> + if (LHS)
> + if (LHS->getLHS()->getExprLoc().isMacroID() ||
> + LHS->getRHS()->getExprLoc().isMacroID())
> + return;
> + if (RHS)
> + if (RHS->getLHS()->getExprLoc().isMacroID() ||
> + RHS->getRHS()->getExprLoc().isMacroID())
> + return;
> if (!LHS || !RHS) first, then the two macro checks afterwards.
>
> + SourceRange DiagRange(B->getLHS()->getLocStart(),
> B->getRHS()->getLocEnd());
> + S.Diag(B->getExprLoc(), diag::warn_operator_always_true_comparison)
> + << DiagRange << isAlwaysTrue;
> Use B->getSourceRange() instead of creating a new SourceRange.
>
> Same changes to compareBoolWithInt
>
> - if (!isTemplateInstantiation)
> + if (!isTemplateInstantiation) {
> + LogicalErrorsHandler Leh(S);
> + AC.getCFGBuildOptions().Observer = &Leh;
> CheckUnreachable(S, AC);
> + }
> This forces your warning to be dependent on a different and unrelated
> function. Instead, use:
>
> if (Diags.getDiagnosticLevel(diag::warn_operator_always_true_comparison,
> D->getLocStart()) {
> LogicalErrorsHandler LEH(S);
> AC.getCFGBuildOptions().Observer = &LEH;
> AC.getCFG();
> }
>
> Place this before the "Emit delayed diagnostics." comment. CFG builds are
> cached, so the observer must be attached to the first run of getCFG. It
> would also be a good idea to have a reset observer function after just in
> case the CFG is rebuilt later.
>
> +def warn_operator_always_true_comparison : Warning<
> + "overlapping comparisons always evaluate to %select{false|true}0">,
> + InGroup<TautologicalCompareOperator>;
> +def warn_bool_and_int_comparison : Warning<
> + "comparison of a boolean expression with an integer other than 0 or 1">,
> + InGroup<TautologicalCompareOperator>;
> Add DefaultIgnore to these warnings.
>
> def TautologicalOutOfRangeCompare :
> DiagGroup<"tautological-constant-out-of-range-compare">;
> def TautologicalCompare : DiagGroup<"tautological-compare",
> [TautologicalOutOfRangeCompare]>;
> +def TautologicalCompareOperator :
> DiagGroup<"tautological-compare-operator",
> + [TautologicalCompare]>;
>
> TautologicalCompareOperator should be a subgroup of TautologicalCompare,
> not the other way around
>
>
> +// RUN: %clang_cc1 -fsyntax-only -verify -Wunreachable-code %s
> Remove -Wunreachable-code and replace with -Wtautological-compare-operator
> in both tests.
> Add some cases with zero to warn-overlap.c
>
> On Fri, Feb 21, 2014 at 5:48 AM, Anders Rönnholm <
> Anders.Ronnholm at evidente.se<mailto:Anders.Ronnholm at evidente.se>> wrote:
> Pinging this patch.
>
> > -----Original Message-----
> > From: Anders Rönnholm
> > Sent: den 20 december 2013 13:35
> > To: Richard Smith; Richard Trieu
> > Cc: cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
> > Subject: Re: [PATCH] check for Incorrect logic in operator
> >
> > Tanks for your comments. New patch attached.
> >
> > -- We can have some warnings in a group on by default and some off by
> > default, but this does suggest that we'd want a new warning subgroup for
> > this warning.
> > Do you want a new subgroup and how do you create one? I have moved it
> > back to tautological and made a what i think is a subgroup.
> You have made -Wtautogical-compare a subgroup of your warning. Switch it
> so that using -Wtautological-compare will also turn on your warning.
> >
> > -- Instead of looking for an integer literal, see if the expression can
> be
> > evaluated, and use that as the constant value. This will allow things
> like 1+2
> > and -1.
> > I will skip this because it will also generate false positives when it's
> x+1 and
> > can be evaluated.
> What false positives are you seeing? I would think that negative numbers
> should be checked with this warning as well.
> >
> > --Also, does your patch cause the unreachable-code warning to trigger in
> > more cases now?
> > I have tested it on llvm and the patch did not trigger more
> unreachable-code
> > warnings.
> >
> > -- What is going on here?
> > Comments in code, also slight refactoring Basicly it checks whether the
> > expression is always true/false by checking following scenarios.
> > x is less than the smallest literal.
> > x is equal to the smallest literal.
> > x is between smallest and largest literal.
> > x is equal to the largest literal.
> > x is greater than largest literal.
> >
> > For && both RHS and LHS needs to be true or false in every scenario for
> it to
> > be regarded always true/false.
> > For || either RHS and LHS needs to be true or false in every scenario
> for it to
> > be regarded always true/false.
> This refactoring makes the logic much clearer now.
> >
> > -- What's the plan with CFGCallBack? New methods added as needed for
> > each new warning? Does the CFGBuilder need to support multiple
> > CFGCallBack's at once?
> > Yes new methods would have to be added. No i don't see a reason for the
> > need of supporting multiple CFGCallBack's at once.
> >
> > //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
> <tel:%2B46%20%280%2970%20912%2042%2054>
> > E-mail: Anders.Ronnholm at evidente.se<mailto:
> Anders.Ronnholm at evidente.se>
> >
> > www.evidente.se<http://www.evidente.se>
> >
> > ________________________________________
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140326/e6114918/attachment.html>
More information about the cfe-commits
mailing list