Thanks for the feedback Doug and Richard. Committed (with feedback incorporated) in r145845 and r145849.<div><br></div><div>- Lang.<br><br><div class="gmail_quote">On Sat, Dec 3, 2011 at 12:40 PM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div><div>On Dec 1, 2011, at 7:22 PM, Lang Hames wrote:</div><br>
<blockquote type="cite">Oops - left my test case out of the previous patch.</blockquote><div><br></div></div>A few comments:</div><div><br></div><div><div>+def warn_impcast_function_to_bool : Warning<</div><div>+ "in implicit conversion to bool, "</div>
<div>+ "the address of %q0 will always evaluate to 'true'">,</div><div>+ InGroup<BoolConversions>;</div><div><br></div><div>It's a little wordy. How about "address of function %q0 will always evaluate 'true'"?</div>
<div><br></div><div>+ if (D && !D->isWeakImported()) {</div></div><div><div>+ FunctionDecl* F = cast<FunctionDecl>(D);</div><div>+ S.Diag(E->getExprLoc(), diag::warn_impcast_function_to_bool)</div>
<div>+ << F << E->getSourceRange() << SourceRange(CC);</div><div>+ return;</div><div>+ }</div><div><br></div><div>There are other "weak" cases here. Check out the IsWeakDecl predicate in ExprConstant.cpp; I suggest making that available so you can re-use it here.</div>
<div><br></div><div>Otherwise, it looks good. I actually don't want us to diagnose</div><div><br></div><div><span style="white-space:pre-wrap"> </span>if (&f)</div><div><br></div><div>because I expected that it's intentional more often than not (because, for example, "f" might be weak on some systems but not on others).</div>
<div><br></div><div><span style="white-space:pre-wrap"> </span>- Doug</div><div><br></div><blockquote type="cite"><div><div><div>- Lang.<br><br><div class="gmail_quote">On Thu, Dec 1, 2011 at 7:16 PM, Lang Hames <span dir="ltr"><<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Richard,<div><br></div><div>I've added source ranges to the diagnostic, however I'm wary of catching cases like</div>
<div><br></div><div>if (&foo) {}</div><div><br></div><div>That seems too deliberate. For now I'd prefer to stick to the simple case of missing parentheses.</div>
<div><br></div><div>What do you think?</div><div><br></div><div>Cheers,</div><div>Lang.</div><div><div><div><br><div class="gmail_quote">On Mon, Nov 28, 2011 at 3:12 PM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div><div>On Mon, Nov 28, 2011 at 1:38 PM, Lang Hames <span dir="ltr"><<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>></span> wrote:<br>
</div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>
This patch adds a warning for implicit conversion from functions to booleans.<div><br></div><div>Could someone take a look and let me know if anything needs fixing or improving?</div><div><br></div><div>Cheers,</div><div>
Lang. </div>
<br></div></div>_______________________________________________<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><br>
<br></blockquote></div><br><div>Can this be changed to catch cases like this:</div><div><br></div><div>void foo();</div><div>if (&foo) {}</div><div><br></div><div>Also, have you thought about formatting the diagnostic to use the DiagnoseImpCast() helper functions like the other implicit cast warnings? If you don't use it, it would be nice to add E->getSourceRange() to underline the function name.</div>
<div><br></div><div><br></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div><span><func-as-bool-warning-3.patch></span>_______________________________________________<div><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><br></div></blockquote></div><br></div></blockquote></div><br></div>