<div dir="ltr">A few more comments for you.<div><br></div><div><div>+</div><div>+#define mydefine 2</div><div>+</div></div><div>...</div><div><div>+</div><div>+ if ((x < mydefine) <= 10) {}</div><div>+} // no-warning</div>
</div><div><br></div><div>Why have you chosen to ignore warnings on macros?</div><div><br></div><div><div>+std::vector<CFGCallback*> CFGObserver::Observers;</div><div>+void CFGObserver::attach(CFGCallback *Observer) {</div>
<div>+ Observers.push_back(Observer);</div><div>+}</div><div>Add the null pointer check here. Since the notify functions are called more often, it makes sense to check for null once here instead of at every call later.</div>
<div> </div><div>+void CFGObserver::detach(CFGCallback *Observer) {</div><div>+ Observers.erase(std::remove(Observers.begin(), Observers.end(), Observer),</div><div>+ Observers.end());</div><div>+}</div>
<div>Use the one argument erase() here.</div><div>+</div><div>+void CFGObserver::notifyCompareAlwaysTrue(const BinaryOperator* B,</div><div>+ bool IsAlwaysTrue) {</div><div>+ for(std::vector<CFGCallback*>::const_iterator I = Observers.begin();</div>
<div>+ I != Observers.end(); ++I) {</div><div>+ if(*I != 0) {</div><div>Remove null check here.</div><div>+ (*I)->compareAlwaysTrue(B,IsAlwaysTrue);</div><div>+ }</div><div>+ }</div><div>+}</div><div>+</div>
<div>+void CFGObserver::notifyCompareBoolWithInt(const BinaryOperator* B) {</div><div>+ for(std::vector<CFGCallback*>::const_iterator I = Observers.begin();</div><div>+ I != Observers.end(); ++I) {</div><div>+ if(*I != 0) {</div>
<div>And null check here.</div><div>+ (*I)->compareBoolWithInt(B);</div><div>+ }</div><div>+ }</div><div>+}</div><div>+</div></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 ? "true" : "false");</div><div>+ }</div><div>+</div></div>
<div class="gmail_extra">We're trying to not pass strings to the diagnostics. Used %select{false|true}0 in the .td file then just pass isAlwaysTrue here.</div><div class="gmail_extra"><br></div><div class="gmail_extra">
<div class="gmail_extra"> bool isTemplateInstantiation = false;</div><div class="gmail_extra"> if (const FunctionDecl *Function = dyn_cast<FunctionDecl>(D))</div><div class="gmail_extra"> isTemplateInstantiation = Function->isTemplateInstantiation();</div>
<div class="gmail_extra">- if (!isTemplateInstantiation)</div><div class="gmail_extra">- CheckUnreachable(S, AC);</div><div class="gmail_extra">+ if (!isTemplateInstantiation) {</div><div class="gmail_extra">
+ LogicalErrorsHandler Leh(S);</div><div class="gmail_extra">+ CFGObserver::attach(&Leh);</div><div class="gmail_extra">+ CheckUnreachable(S, AC);</div><div class="gmail_extra">+ CFGObserver::detach(&Leh);</div>
<div class="gmail_extra">+ }</div><div class="gmail_extra"> }</div><div class="gmail_extra">I assume this is why -Wunreachable-code is needed in the tests. This introduces a dependency which gives different warnings for -Wtautological-compare depending on an unrelated warning flag. Consider running a separate CFG pass for warnings. Also, this somehow got indented an extra two spaces.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_extra"> def warn_lunsigned_always_true_comparison : Warning<</div><div class="gmail_extra"> "comparison of unsigned%select{| enum}2 expression %0 is always %1">,</div>
<div class="gmail_extra"> InGroup<TautologicalCompare>;</div><div class="gmail_extra">+def warn_operator_always_true_comparison : Warning<</div><div class="gmail_extra">+ "logical disjunction always evaluates to %0">,</div>
<div class="gmail_extra">+ InGroup<TautologicalCompare>;</div><div class="gmail_extra">+def warn_bool_and_int_comparison : Warning<</div><div class="gmail_extra">+ "comparison of a boolean expression with an integer other than 0 or 1">,</div>
<div class="gmail_extra">+ InGroup<TautologicalCompare>;</div><div class="gmail_extra"> def warn_out_of_range_compare : Warning<</div><div class="gmail_extra"> "comparison of constant %0 with expression of type %1 is always "</div>
<div class="gmail_extra"> "%select{false|true}2">, InGroup<TautologicalOutOfRangeCompare>;</div></div>def warn_bool_and_int_comparison should be in TautologicalOutOfRangeCompare instead of TautologicalCompare.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_extra">+/// \brief Keeps track of all observers. Provides functionality to add and</div><div class="gmail_extra">+/// remove observers and notify observers on found logical operator errors.</div>
<div class="gmail_extra">+class CFGObserver {</div><div class="gmail_extra">+public:</div><div class="gmail_extra">+ static void attach(CFGCallback *Observer);</div><div class="gmail_extra">+ static void detach(CFGCallback *Observer);</div>
<div class="gmail_extra">+ static void notifyCompareAlwaysTrue(const BinaryOperator* B,</div><div class="gmail_extra">+ bool IsAlwaysTrue);</div><div class="gmail_extra">+ static void notifyCompareBoolWithInt(const BinaryOperator* B);</div>
<div class="gmail_extra">+private:</div><div class="gmail_extra">+ static std::vector<CFGCallback*> Observers;</div><div class="gmail_extra">+};</div><div class="gmail_extra">+</div><div class="gmail_extra">I don't like the idea of essentially a global vector of callbacks for the CFG. Perhaps passing a vector to the CFG builder would be a better way.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">That's it for the non-CFG code. I'll see if I can look at that later.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Oct 28, 2013 at 1:55 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">
<div lang="SV" link="blue" vlink="purple">
<div>
<p class="MsoNormal" style="margin-bottom:12pt"><span lang="EN-US" style="font-size:10pt;font-family:Tahoma,sans-serif">Thanks for your comments.<u></u><u></u></span></p>
<p class="MsoNormal" style="margin-bottom:12pt"><span lang="EN-US" style="font-size:10pt;font-family:Tahoma,sans-serif">I was told that it was preferable to update the CFG. The reason is that all analyses built on top on the CFG will benefit:
-Wuninitialized, static analyzer, warnings, etc. I wonder if Anna or Ted could provide their opinions about this patch?<u></u><u></u></span></p>
<p class="MsoNormal" style="margin-bottom:12pt"><span lang="EN-US" style="font-size:10pt;font-family:Tahoma,sans-serif">//Anders<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u> <u></u></span></p>
<p class="MsoNormal"><b><span lang="EN-US" style="font-size:10pt;font-family:Tahoma,sans-serif">From:</span></b><span lang="EN-US" style="font-size:10pt;font-family:Tahoma,sans-serif"> Richard Trieu [mailto:<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>]
<br>
<b>Sent:</b> den 26 oktober 2013 01:43<br>
<b>To:</b> Jordan Rose<br>
<b>Cc:</b> Anders Rönnholm; <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<b>Subject:</b> Re: [PATCH] check for Incorrect logic in operator<u></u><u></u></span></p><div><div class="h5">
<p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p>
<div>
<p class="MsoNormal">A few high level comments.<u></u><u></u></p>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">"-fblocks -Wunreachable-code" is there any reason your test cases have these flags?<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">"if (!x == 10) {}" there is an existing warning for this called -Wlogical-not-parentheses. The check should be folded into there.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">"if ((x < y) == 10) {}" ditto this and -Wtautological-compare-out-of-range<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">"logical disjunction always evaluates to true" I wouldn't expect all programmers to understand what a logical disjunction is.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">If you check out -Wlogical-not-parentheses and -Wtautological-compare-out-of-range, they are in lib/Sema/SemaChecking.cpp. Why did you use a CFG approach instead?<u></u><u></u></p>
</div>
</div>
<div>
<p class="MsoNormal" style="margin-bottom:12pt"><u></u> <u></u></p>
<div>
<p class="MsoNormal">On Fri, Oct 25, 2013 at 10:18 AM, Jordan Rose <<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>> wrote:<u></u><u></u></p>
<p class="MsoNormal">Richard Trieu has worked on a number of similar warnings. Maybe he can take a look?<br>
<br>
Jordan<u></u><u></u></p>
<div>
<div>
<p class="MsoNormal"><br>
<br>
On Oct 25, 2013, at 8:18, Anders Rönnholm <<a href="mailto:Anders.Ronnholm@evidente.se" target="_blank">Anders.Ronnholm@evidente.se</a>> wrote:<br>
<br>
> Hi,<br>
> Resending the patch I sent last week. Anyone want to have a look at it?<br>
><br>
> //Anders<br>
><br>
> -----Original Message-----<br>
> From: Anders Rönnholm<br>
> Sent: den 18 oktober 2013 16:05<br>
> To: <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
> Subject: [PATCH] check for Incorrect logic in operator<br>
><br>
> Hi,<br>
><br>
> I have a new patch I like to get reviewed. It checks for incorrect logics in relational,equal and logic operators in cfgbuilder when trying to evaluate bool. A warning is emitted in Sema when an error occurs.<br>
><br>
> eg.<br>
> if (x != 2 || x != 3) { } always true<br>
> if ((x < y) <= 10) {} always true<br>
><br>
> Thanks,<br>
> Anders<u></u><u></u></p>
</div>
</div>
<p class="MsoNormal" style="margin-bottom:12pt">> <incorrectoperatorlogic.diff>_______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><u></u><u></u></p>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
</div></div></div>
</div>
</blockquote></div><br></div></div>