patch for warning on logical negation with known result

[Richard Trieu]

On Thu, Oct 23, 2014 at 10:24 AM, jahanian <fjahanian at apple.com> wrote:

> 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)…"
> Note that this is a c-only patch. c++ already has a warning when such
> non-null pointers are implicitly converted to bool.
> Is this ok for check-in?
>
> - Fariborz
>
>
 Index: include/clang/AST/Expr.h
===================================================================
--- include/clang/AST/Expr.h (revision 220486)
+++ include/clang/AST/Expr.h (working copy)
@@ -641,7 +641,10 @@
     NPCK_CXX11_nullptr,

     /// \brief Expression is a GNU-style __null constant.
-    NPCK_GNUNull
+    NPCK_GNUNull,
+
+    /// \brief Expression is operand of a logical negation operator
+    NPCK_LNEG_ZeroLiteral
   };

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.

Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td (revision 220486)
+++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
@@ -2512,6 +2512,10 @@
     "comparison of %select{address of|function|array}0 '%1' %select{not
|}2"
     "equal to a null pointer is always %select{true|false}2">,
     InGroup<TautologicalPointerCompare>;
+def warn_null_logical_negation_compare : Warning<
+    "logical negation of %select{address of|function|array}0 '%1' is "
+    "always %select{true|false}2">,
+    InGroup<TautologicalPointerCompare>;
 def warn_this_null_compare : Warning<
   "'this' pointer cannot be null in well-defined C++ code; comparison may
be "
   "assumed to always evaluate to %select{true|false}0">,

There should not be two different messages for the same warning in C versus
C++ mode.  Use the old diagnostic message to consistent.

Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp (revision 220486)
+++ lib/Sema/SemaExpr.cpp (working copy)
@@ -9952,7 +9952,10 @@
   // that are explicitly defined as valid by the standard).
   if (Opc != UO_AddrOf && Opc != UO_Deref)
     CheckArrayAccess(Input.get());
-
+  if (Opc == UO_LNot && Input.get()->getType()->isPointerType() &&
+      !getLangOpts().CPlusPlus)
+    DiagnoseAlwaysNonNullPointer(Input.get(), Expr::NPCK_LNEG_ZeroLiteral,
+                                 true, SourceRange());
   return new (Context)
       UnaryOperator(Input.get(), Opc, resultType, VK, OK, OpLoc);
 }

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141023/7768a368/attachment.html>