<div dir="ltr">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.<div>
<br></div><div><div>+ if(BuildOpts.Observer)</div><div>+ BuildOpts.Observer->compareBoolWithInt(B);</div><div>Add space after 'if'</div><div><br></div><div>TautologicalCompareOperator isn't very descriptive. Once the bool compares are removed, I think TautologicalOverlapCompare would be a better name for it.<br>
</div><div class="gmail_extra"><br>
<div class="gmail_quote">On Fri, Mar 14, 2014 at 7:51 AM, Anders Rönnholm <span dir="ltr"><<a href="mailto:Anders.Ronnholm@evidente.se" target="_blank">Anders.Ronnholm@evidente.se</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Hi Richard,<br>
<br>
Modified the patch according to your comments.<br>
<br>
Except for Use the APSInt ++operator as i don't want to increment L1 or L2.<br>
<br>
//Anders<br>
<br>
.......................................................................................................................<br>
<div>Anders Rönnholm Senior Engineer<br>
Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden<br>
<br>
Mobile: <a href="tel:%2B46%20%280%2970%20912%2042%2054" value="+46709124254" target="_blank">+46 (0)70 912 42 54</a><br>
E-mail: <a href="mailto:Anders.Ronnholm@evidente.se" target="_blank">Anders.Ronnholm@evidente.se</a><br>
<br>
<a href="http://www.evidente.se" target="_blank">www.evidente.se</a><br>
<br>
________________________________________<br>
</div>Från: Richard Trieu [<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>]<br>
Skickat: den 6 mars 2014 06:25<br>
Till: Anders Rönnholm<br>
Cc: Richard Smith; <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
Ämne: Re: [PATCH] check for Incorrect logic in operator<br>
<div><div><br>
+ const IntegerLiteral *Literal1 =<br>
+ dyn_cast<IntegerLiteral>(LHS->getRHS()->IgnoreParens());<br>
Note that this won't work on negative values.<br>
<br>
+ Literal1->EvaluateAsInt(L1,*Context);<br>
+ Literal2->EvaluateAsInt(L2,*Context);<br>
+<br>
+ if (!L1 || !L2)<br>
+ return TryResult();<br>
if (!Literal1->EvaluateAsInt(L1, *Context) || Literal2->EvaluateAsInt(L2, *Context))<br>
return TryResult();<br>
<br>
+ const llvm::APSInt MinValue =<br>
+ llvm::APSInt::getMinValue(L1.getBitWidth(), L1.isUnsigned());<br>
+ const llvm::APSInt MaxValue =<br>
+ llvm::APSInt::getMaxValue(L1.getBitWidth(), L1.isUnsigned());<br>
Move these into the array below.<br>
<br>
+ llvm::APSInt I1 = llvm::APSInt(llvm::APInt(L1.getBitWidth(), 1),<br>
+ L1.isUnsigned());<br>
Don't need this.<br>
<br>
+ // Value between Value1 and Value2<br>
+ ((L1 < L2) ? L1 : L2) + I1,<br>
Use the APSInt ++operator.<br>
<br>
+ if(AlwaysTrue || AlwaysFalse) {<br>
+ if(BuildOpts.Observer)<br>
Add space between the if and the parentheses.<br>
<br>
+ void compareAlwaysTrue(const BinaryOperator* B, bool isAlwaysTrue) {<br>
BinaryOperator *B<br>
<br>
+ if (LHS)<br>
+ if (LHS->getLHS()->getExprLoc().isMacroID() ||<br>
+ LHS->getRHS()->getExprLoc().isMacroID())<br>
+ return;<br>
+ if (RHS)<br>
+ if (RHS->getLHS()->getExprLoc().isMacroID() ||<br>
+ RHS->getRHS()->getExprLoc().isMacroID())<br>
+ return;<br>
if (!LHS || !RHS) first, then the two macro checks afterwards.<br>
<br>
+ SourceRange DiagRange(B->getLHS()->getLocStart(), B->getRHS()->getLocEnd());<br>
+ S.Diag(B->getExprLoc(), diag::warn_operator_always_true_comparison)<br>
+ << DiagRange << isAlwaysTrue;<br>
Use B->getSourceRange() instead of creating a new SourceRange.<br>
<br>
Same changes to compareBoolWithInt<br>
<br>
- if (!isTemplateInstantiation)<br>
+ if (!isTemplateInstantiation) {<br>
+ LogicalErrorsHandler Leh(S);<br>
+ AC.getCFGBuildOptions().Observer = &Leh;<br>
CheckUnreachable(S, AC);<br>
+ }<br>
This forces your warning to be dependent on a different and unrelated function. Instead, use:<br>
<br>
if (Diags.getDiagnosticLevel(diag::warn_operator_always_true_comparison, D->getLocStart()) {<br>
LogicalErrorsHandler LEH(S);<br>
AC.getCFGBuildOptions().Observer = &LEH;<br>
AC.getCFG();<br>
}<br>
<br>
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.<br>
<br>
+def warn_operator_always_true_comparison : Warning<<br>
+ "overlapping comparisons always evaluate to %select{false|true}0">,<br>
+ InGroup<TautologicalCompareOperator>;<br>
+def warn_bool_and_int_comparison : Warning<<br>
+ "comparison of a boolean expression with an integer other than 0 or 1">,<br>
+ InGroup<TautologicalCompareOperator>;<br>
Add DefaultIgnore to these warnings.<br>
<br>
def TautologicalOutOfRangeCompare : DiagGroup<"tautological-constant-out-of-range-compare">;<br>
def TautologicalCompare : DiagGroup<"tautological-compare",<br>
[TautologicalOutOfRangeCompare]>;<br>
+def TautologicalCompareOperator : DiagGroup<"tautological-compare-operator",<br>
+ [TautologicalCompare]>;<br>
<br>
TautologicalCompareOperator should be a subgroup of TautologicalCompare, not the other way around<br>
<br>
<br>
+// RUN: %clang_cc1 -fsyntax-only -verify -Wunreachable-code %s<br>
Remove -Wunreachable-code and replace with -Wtautological-compare-operator in both tests.<br>
Add some cases with zero to warn-overlap.c<br>
<br>
</div></div><div>On Fri, Feb 21, 2014 at 5:48 AM, Anders Rönnholm <<a href="mailto:Anders.Ronnholm@evidente.se" target="_blank">Anders.Ronnholm@evidente.se</a><mailto:<a href="mailto:Anders.Ronnholm@evidente.se" target="_blank">Anders.Ronnholm@evidente.se</a>>> wrote:<br>
Pinging this patch.<br>
<br>
> -----Original Message-----<br>
> From: Anders Rönnholm<br>
> Sent: den 20 december 2013 13:35<br>
> To: Richard Smith; Richard Trieu<br>
</div><div><div>> Cc: <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><mailto:<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a>><br>
> Subject: Re: [PATCH] check for Incorrect logic in operator<br>
><br>
> Tanks for your comments. New patch attached.<br>
><br>
> -- We can have some warnings in a group on by default and some off by<br>
> default, but this does suggest that we'd want a new warning subgroup for<br>
> this warning.<br>
> Do you want a new subgroup and how do you create one? I have moved it<br>
> back to tautological and made a what i think is a subgroup.<br>
You have made -Wtautogical-compare a subgroup of your warning. Switch it so that using -Wtautological-compare will also turn on your warning.<br>
><br>
> -- Instead of looking for an integer literal, see if the expression can be<br>
> evaluated, and use that as the constant value. This will allow things like 1+2<br>
> and -1.<br>
> I will skip this because it will also generate false positives when it's x+1 and<br>
> can be evaluated.<br>
What false positives are you seeing? I would think that negative numbers should be checked with this warning as well.<br>
><br>
> --Also, does your patch cause the unreachable-code warning to trigger in<br>
> more cases now?<br>
> I have tested it on llvm and the patch did not trigger more unreachable-code<br>
> warnings.<br>
><br>
> -- What is going on here?<br>
> Comments in code, also slight refactoring Basicly it checks whether the<br>
> expression is always true/false by checking following scenarios.<br>
> x is less than the smallest literal.<br>
> x is equal to the smallest literal.<br>
> x is between smallest and largest literal.<br>
> x is equal to the largest literal.<br>
> x is greater than largest literal.<br>
><br>
> For && both RHS and LHS needs to be true or false in every scenario for it to<br>
> be regarded always true/false.<br>
> For || either RHS and LHS needs to be true or false in every scenario for it to<br>
> be regarded always true/false.<br>
This refactoring makes the logic much clearer now.<br>
><br>
> -- What's the plan with CFGCallBack? New methods added as needed for<br>
> each new warning? Does the CFGBuilder need to support multiple<br>
> CFGCallBack's at once?<br>
> Yes new methods would have to be added. No i don't see a reason for the<br>
> need of supporting multiple CFGCallBack's at once.<br>
><br>
> //Anders<br>
><br>
><br>
> .....................................................................................................................<br>
> ..<br>
> Anders Rönnholm Senior Engineer<br>
> Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden<br>
><br>
</div></div>> Mobile: <a href="tel:%2B46%20%280%2970%20912%2042%2054" value="+46709124254" target="_blank">+46 (0)70 912 42 54</a><tel:%2B46%20%280%2970%20912%2042%2054><br>
> E-mail: <a href="mailto:Anders.Ronnholm@evidente.se" target="_blank">Anders.Ronnholm@evidente.se</a><mailto:<a href="mailto:Anders.Ronnholm@evidente.se" target="_blank">Anders.Ronnholm@evidente.se</a>><br>
><br>
> <a href="http://www.evidente.se" target="_blank">www.evidente.se</a><<a href="http://www.evidente.se" target="_blank">http://www.evidente.se</a>><br>
><br>
> ________________________________________</blockquote></div><br></div></div></div>