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 < [email protected]> 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: [email protected] > > www.evidente.se > > ________________________________________ > Från: Richard Trieu [[email protected]] > Skickat: den 6 mars 2014 06:25 > Till: Anders Rönnholm > Cc: Richard Smith; [email protected] > Ä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 < > [email protected]<mailto:[email protected]>> wrote: > Pinging this patch. > > > -----Original Message----- > > From: Anders Rönnholm > > Sent: den 20 december 2013 13:35 > > To: Richard Smith; Richard Trieu > > Cc: [email protected]<mailto:[email protected]> > > 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: [email protected]<mailto: > [email protected]> > > > > www.evidente.se<http://www.evidente.se> > > > > ________________________________________
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
