<div dir="ltr"><div><div><div>+ const IntegerLiteral *Literal1 =</div><div>+ dyn_cast<IntegerLiteral>(LHS->getRHS()->IgnoreParens());</div></div><div>Note that this won't work on negative values.</div>
<div><br></div><div>+ Literal1->EvaluateAsInt(L1,*Context);<br></div></div><div><div>+ Literal2->EvaluateAsInt(L2,*Context);</div><div>+</div><div>+ if (!L1 || !L2)</div><div>+ return TryResult();</div>
</div><div>if (!Literal1->EvaluateAsInt(L1, *Context) || Literal2->EvaluateAsInt(L2, *Context))</div><div> return TryResult();</div><div><br></div><div><div>+ const llvm::APSInt MinValue =</div><div>+ llvm::APSInt::getMinValue(L1.getBitWidth(), L1.isUnsigned());</div>
<div>+ const llvm::APSInt MaxValue =</div><div>+ llvm::APSInt::getMaxValue(L1.getBitWidth(), L1.isUnsigned());</div></div><div>Move these into the array below.</div><div><br></div><div><div>+ llvm::APSInt I1 = llvm::APSInt(llvm::APInt(L1.getBitWidth(), 1),</div>
<div>+ L1.isUnsigned());</div></div><div>Don't need this.</div><div><br></div><div><div>+ // Value between Value1 and Value2</div><div>+ ((L1 < L2) ? L1 : L2) + I1,</div></div><div>Use the APSInt ++operator.</div>
<div><br></div><div><div>+ if(AlwaysTrue || AlwaysFalse) {</div><div>+ if(BuildOpts.Observer)</div></div><div>Add space between the if and the parentheses.</div><div><br></div><div>+ void compareAlwaysTrue(const BinaryOperator* B, bool isAlwaysTrue) {<br>
</div><div>BinaryOperator *B</div><div><br></div><div><div>+ if (LHS)</div><div>+ if (LHS->getLHS()->getExprLoc().isMacroID() ||</div><div>+ LHS->getRHS()->getExprLoc().isMacroID())</div><div>
+ return;</div><div>+ if (RHS)</div><div>+ if (RHS->getLHS()->getExprLoc().isMacroID() ||</div><div>+ RHS->getRHS()->getExprLoc().isMacroID())</div><div>+ return;</div></div>
<div>if (!LHS || !RHS) first, then the two macro checks afterwards.</div><div><br></div><div><div>+ SourceRange DiagRange(B->getLHS()->getLocStart(), B->getRHS()->getLocEnd());</div><div>+ S.Diag(B->getExprLoc(), diag::warn_operator_always_true_comparison)</div>
<div>+ << DiagRange << isAlwaysTrue;</div></div><div>Use B->getSourceRange() instead of creating a new SourceRange.</div><div><br></div><div>Same changes to compareBoolWithInt</div><div><br></div><div>
<div>- if (!isTemplateInstantiation)</div><div>+ if (!isTemplateInstantiation) {</div><div>+ LogicalErrorsHandler Leh(S);</div><div>+ AC.getCFGBuildOptions().Observer = &Leh;</div><div> CheckUnreachable(S, AC);</div>
<div>+ }</div></div><div>This forces your warning to be dependent on a different and unrelated function. Instead, use:</div><div><br></div><div><div>if (Diags.getDiagnosticLevel(diag::warn_operator_always_true_comparison, D->getLocStart()) {</div>
</div><div><div> LogicalErrorsHandler LEH(S);</div><div> AC.getCFGBuildOptions().Observer = &LEH;</div></div><div> AC.getCFG();</div><div>}</div><div><br></div><div>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.</div>
<div><br></div><div>+def warn_operator_always_true_comparison : Warning<</div><div>+ "overlapping comparisons always evaluate to %select{false|true}0">,</div><div>+ InGroup<TautologicalCompareOperator>;</div>
<div>+def warn_bool_and_int_comparison : Warning<</div><div>+ "comparison of a boolean expression with an integer other than 0 or 1">,</div><div>+ InGroup<TautologicalCompareOperator>;</div><div>Add DefaultIgnore to these warnings.<br>
</div><div><br></div><div>def TautologicalOutOfRangeCompare : DiagGroup<"tautological-constant-out-of-range-compare">;<br></div><div>def TautologicalCompare : DiagGroup<"tautological-compare",</div>
<div> [TautologicalOutOfRangeCompare]>;</div><div>+def TautologicalCompareOperator : DiagGroup<"tautological-compare-operator",</div><div>+ [TautologicalCompare]>;</div>
<div><br></div><div>TautologicalCompareOperator should be a subgroup of TautologicalCompare, not the other way around<br></div><div class="gmail_extra"><br><br>+// RUN: %clang_cc1 -fsyntax-only -verify -Wunreachable-code %s</div>
<div class="gmail_extra">Remove -Wunreachable-code and replace with -Wtautological-compare-operator in both tests.</div><div class="gmail_extra">Add some cases with zero to warn-overlap.c</div><div class="gmail_extra"><br>
<div class="gmail_quote">On Fri, Feb 21, 2014 at 5:48 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">
Pinging this patch.<br>
<div class=""><br>
> -----Original Message-----<br>
> From: Anders Rönnholm<br>
</div><div><div class="h5">> Sent: den 20 december 2013 13:35<br>
> To: Richard Smith; Richard Trieu<br>
> Cc: <a href="mailto:cfe-commits@cs.uiuc.edu">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></div></div></blockquote><div>You have made -Wtautogical-compare a subgroup of your warning. Switch it so that using -Wtautological-compare will also turn on your warning. </div>
<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"><div><div class="h5">
><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></div></div></blockquote><div>What false positives are you seeing? I would think that negative numbers should be checked with this warning as well. </div><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">
<div><div class="h5">
><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></div></div></blockquote><div>This refactoring makes the logic much clearer now. </div><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">
<div><div class="h5">
><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>
> Mobile: <a href="tel:%2B46%20%280%2970%20912%2042%2054" value="+46709124254">+46 (0)70 912 42 54</a><br>
> E-mail: <a href="mailto:Anders.Ronnholm@evidente.se">Anders.Ronnholm@evidente.se</a><br>
><br>
> <a href="http://www.evidente.se" target="_blank">www.evidente.se</a><br>
><br>
> ________________________________________</div></div></blockquote></div></div></div>