SV: [PATCH] check for Incorrect logic in operator
Anders Rönnholm
Anders.Ronnholm at evidente.se
Wed Apr 2 10:11:18 PDT 2014
Yes we seem to warn on the same things, strange that we didn't notice that.
Removed my warning and changed name.
//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 04:28
Till: Anders Rönnholm
Cc: Richard Smith; cfe-commits at cs.uiuc.edu
Ämne: Re: [PATCH] check for Incorrect logic in operator
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<mailto: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<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>
________________________________________
Från: Richard Trieu [rtrieu at google.com<mailto:rtrieu at google.com>]
Skickat: den 6 mars 2014 06:25
Till: Anders Rönnholm
Cc: Richard Smith; cfe-commits at cs.uiuc.edu<mailto: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><mailto: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><mailto: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><tel:%2B46%20%280%2970%20912%2042%2054>
> E-mail: Anders.Ronnholm at evidente.se<mailto:Anders.Ronnholm at evidente.se><mailto:Anders.Ronnholm at evidente.se<mailto:Anders.Ronnholm at evidente.se>>
>
> www.evidente.se<http://www.evidente.se><http://www.evidente.se>
>
> ________________________________________
-------------- next part --------------
A non-text attachment was scrubbed...
Name: incorrectlogic.diff
Type: text/x-patch
Size: 17492 bytes
Desc: incorrectlogic.diff
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140402/8779b876/attachment.bin>
More information about the cfe-commits
mailing list