<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Oct 23, 2014 at 10:24 AM, jahanian <span dir="ltr"><<a href="mailto:fjahanian@apple.com" target="_blank">fjahanian@apple.com</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">I would like to warn on logical negation of operands with known non-null value; as in "if (!array)…", just as we warn on "if (array == 0)…"<br>
Note that this is a c-only patch. c++ already has a warning when such non-null pointers are implicitly converted to bool.<br>
Is this ok for check-in?<br>
<br>
- Fariborz<br><br></blockquote><div><br></div><div> Index: include/clang/AST/Expr.h</div><div>===================================================================</div><div>--- include/clang/AST/Expr.h<span class="" style="white-space:pre">    </span>(revision 220486)</div><div>+++ include/clang/AST/Expr.h<span class="" style="white-space:pre">      </span>(working copy)</div><div>@@ -641,7 +641,10 @@</div><div>     NPCK_CXX11_nullptr,</div><div> </div><div>     /// \brief Expression is a GNU-style __null constant.</div><div>-    NPCK_GNUNull</div><div>+    NPCK_GNUNull,</div><div>+    </div><div>+    /// \brief Expression is operand of a logical negation operator</div><div>+    NPCK_LNEG_ZeroLiteral</div><div>   };</div><div><br></div><div>Don't add another element to enum NullPointerConstantKind.  This is used for determining what kind of null pointer an expression is, meaning it should only be extended if the evaluate for null functions in Expr are updated to return it.  Requiring another value for a parameter in Sema function is not a good reason to extend the enum.<br></div><div><br></div><div><div>Index: include/clang/Basic/DiagnosticSemaKinds.td</div><div>===================================================================</div><div>--- include/clang/Basic/DiagnosticSemaKinds.td<span class="" style="white-space:pre">       </span>(revision 220486)</div><div>+++ include/clang/Basic/DiagnosticSemaKinds.td<span class="" style="white-space:pre">    </span>(working copy)</div><div>@@ -2512,6 +2512,10 @@</div><div>     "comparison of %select{address of|function|array}0 '%1' %select{not |}2"</div><div>     "equal to a null pointer is always %select{true|false}2">,</div><div>     InGroup<TautologicalPointerCompare>;</div><div>+def warn_null_logical_negation_compare : Warning<</div><div>+    "logical negation of %select{address of|function|array}0 '%1' is "</div><div>+    "always %select{true|false}2">,</div><div>+    InGroup<TautologicalPointerCompare>;</div><div> def warn_this_null_compare : Warning<</div><div>   "'this' pointer cannot be null in well-defined C++ code; comparison may be "</div><div>   "assumed to always evaluate to %select{true|false}0">,</div></div><div><br></div><div>There should not be two different messages for the same warning in C versus C++ mode.  Use the old diagnostic message to consistent.</div><div><br></div><div><div>Index: lib/Sema/SemaExpr.cpp</div><div>===================================================================</div><div>--- lib/Sema/SemaExpr.cpp<span class="" style="white-space:pre">   </span>(revision 220486)</div><div>+++ lib/Sema/SemaExpr.cpp<span class="" style="white-space:pre"> </span>(working copy)</div><div>@@ -9952,7 +9952,10 @@</div><div>   // that are explicitly defined as valid by the standard).</div><div>   if (Opc != UO_AddrOf && Opc != UO_Deref)</div><div>     CheckArrayAccess(Input.get());</div><div>-</div><div>+  if (Opc == UO_LNot && Input.get()->getType()->isPointerType() &&</div><div>+      !getLangOpts().CPlusPlus)</div><div>+    DiagnoseAlwaysNonNullPointer(Input.get(), Expr::NPCK_LNEG_ZeroLiteral,</div><div>+                                 true, SourceRange());</div><div>   return new (Context)</div><div>       UnaryOperator(Input.get(), Opc, resultType, VK, OK, OpLoc);</div><div> }</div></div><div><br></div><div>DiagnoseAlwaysNonNullPointer is currently called from the CheckImplicitConversion and AnalyzeImplicitConversion set of functions.  A better way is to find where a logical not expression is processed during the implicit conversion checks, then add a special case to check the sub-expression for conversions to bool.  Also, the LangOptions should be checked for Bool, not CPlusPlus, and extra checks for logical and and logical or should happen as well.</div></div></div></div>