Re: patch for warning on logical negation with known result

jahanian Fri, 24 Oct 2014 13:25:10 -0700

Thanks for the review. Here is the updated patch.

Index: include/clang/Sema/Sema.h
===================================================================
--- include/clang/Sema/Sema.h   (revision 220567)
+++ include/clang/Sema/Sema.h   (working copy)
@@ -7975,6 +7975,7 @@
   void DiagnoseAlwaysNonNullPointer(Expr *E,
                                     Expr::NullPointerConstantKind NullType,
                                     bool IsEqual, SourceRange Range);
+  void CheckAlwaysNonNullPointer(Expr *OrigExp);
 
   /// type checking for vector binary operators.
   QualType CheckVectorOperands(ExprResult &LHS, ExprResult &RHS,
Index: lib/Sema/SemaChecking.cpp
===================================================================
--- lib/Sema/SemaChecking.cpp   (revision 220567)
+++ lib/Sema/SemaChecking.cpp   (working copy)
@@ -6778,6 +6778,21 @@
       << FixItHint::CreateInsertion(getLocForEndOfToken(E->getLocEnd()), "()");
 }
 
+void Sema::CheckAlwaysNonNullPointer(Expr *OrigExpr) {
+  if (const UnaryOperator *U = dyn_cast<UnaryOperator>(OrigExpr))
+    if (U->getOpcode() == UO_LNot)
+      return CheckAlwaysNonNullPointer(U->getSubExpr());
+    
+  Expr *E = OrigExpr->IgnoreParenImpCasts();
+  QualType Target = OrigExpr->getType();
+  QualType Source = E->getType();
+  if (Source == Target)
+    return;
+  if (Target->isPointerType() &&
+      (Source->isPointerType() || Source->canDecayToPointerType()))
+    DiagnoseAlwaysNonNullPointer(E, Expr::NPCK_NotNull, /*IsEqual*/ false,
+                                 SourceRange());
+}
 
 /// Diagnoses "dangerous" implicit conversions within the given
 /// expression (which is a full expression).  Implements -Wconversion
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp       (revision 220567)
+++ lib/Sema/SemaExpr.cpp       (working copy)
@@ -8412,7 +8412,10 @@
     if (!LHS.get()->getType()->isScalarType() ||
         !RHS.get()->getType()->isScalarType())
       return InvalidOperands(Loc, LHS, RHS);
-
+    
+    CheckAlwaysNonNullPointer(LHS.get());
+    CheckAlwaysNonNullPointer(RHS.get());
+    
     return Context.IntTy;
   }
 
@@ -12964,6 +12967,7 @@
         << T << E->getSourceRange();
       return ExprError();
     }
+    CheckAlwaysNonNullPointer(E);
   }
 
   return E;
Index: test/Analysis/logical-ops.c
===================================================================
--- test/Analysis/logical-ops.c (revision 220567)
+++ test/Analysis/logical-ops.c (working copy)
@@ -36,4 +36,5 @@
 int undef(void) {} // expected-warning{{control reaches end of non-void 
function}}
 void useUndef(void) { 0 || undef(); }
 
-void testPointer(void) { (void) (1 && testPointer && 0); }
+void testPointer(void) { (void) (1 && testPointer && 0); } // expected-warning 
{{address of function 'testPointer' will always evaluate to 'true'}} \
+                                                          // expected-note 
{{prefix with the address-of operator to silence this warning}}
Index: test/Sema/exprs.c
===================================================================
--- test/Sema/exprs.c   (revision 220567)
+++ test/Sema/exprs.c   (working copy)
@@ -244,6 +244,10 @@
   if ("help")
     (void) 0;
 
-  if (test22)
+  if (test22) // expected-warning {{address of function 'test22' will always 
evaluate to 'true'}} \
+             // expected-note {{prefix with the address-of operator to silence 
this warning}}
     (void) 0;
+
+  if (&test22)
+    (void) 0;
 }
Index: test/Sema/warn-tautological-compare.c
===================================================================
--- test/Sema/warn-tautological-compare.c       (revision 0)
+++ test/Sema/warn-tautological-compare.c       (working copy)
@@ -0,0 +1,77 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -fsyntax-only -verify  %s
+// rdar://18716393
+
+extern int a[] __attribute__((weak));
+int b[] = {8,13,21};
+struct {
+  int x[10];
+} c;
+const char str[] = "text";
+
+void ignore() {
+  if (!a) {}
+}
+void test() {
+  if (!b) {} // expected-warning {{address of array 'b' will always evaluate 
to 'true'}}
+  if (b == 0) {} // expected-warning {{comparison of array 'b' equal to a null 
pointer is always false}}
+  if (!c.x) {} // expected-warning {{address of array 'c.x' will always 
evaluate to 'true'}}
+  if (c.x == 0) {} // expected-warning {{comparison of array 'c.x' equal to a 
null pointer is always false}}
+  if (!str) {} // expected-warning {{address of array 'str' will always 
evaluate to 'true'}}
+  if (0 == str) {} // expected-warning {{comparison of array 'str' equal to a 
null pointer is always false}}
+}
+
+int array[2];
+int test1()
+{
+  if (!array) { // expected-warning {{address of array 'array' will always 
evaluate to 'true'}}
+    return array[0];
+  } else if (array != 0) { // expected-warning {{comparison of array 'array' 
not equal to a null pointer is always true}}
+    return array[1];
+  }
+  if (array == 0) // expected-warning {{comparison of array 'array' equal to a 
null pointer is always false}}
+    return 1;
+  return 0;
+}
+
+#define NULL (void*)0
+
+int test2(int* pointer, char ch, void * pv) __attribute__((nonnull(1, 3)));
+int test2(int* pointer, char ch, void * pv) {
+   if (!pointer) { 
+     return 0;
+   }
+
+   if (pointer == NULL) {} // expected-warning {{comparison of nonnull 
parameter 'pointer' equal to a null pointer is always false}}
+
+   if (pointer != NULL) {} // expected-warning {{comparison of nonnull 
parameter 'pointer' not equal to a null pointer is always true}}
+
+   return 1;
+}
+
+void test3() {
+   if (array) { } // expected-warning {{address of array 'array' will always 
evaluate to 'true'}}
+   if (array != 0) {} // expected-warning {{comparison of array 'array' not 
equal to a null pointer is always true}}
+   if (!array) { } // expected-warning {{address of array 'array' will always 
evaluate to 'true'}}
+   if (array == 0) {} // expected-warning {{comparison of array 'array' equal 
to a null pointer is always false}}
+
+   if (array[0] &&
+       array) {} // expected-warning {{address of array 'array' will always 
evaluate to 'true'}}
+
+   if (array[0] ||
+       array) {} // expected-warning {{address of array 'array' will always 
evaluate to 'true'}}
+
+   if (array[0] &&
+       !array) {} // expected-warning {{address of array 'array' will always 
evaluate to 'true'}}
+   if (array[0] ||
+       !array) {} // expected-warning {{address of array 'array' will always 
evaluate to 'true'}}
+
+   if (array && // expected-warning {{address of array 'array' will always 
evaluate to 'true'}}
+       array[0]) {}
+   if (!array || // expected-warning {{address of array 'array' will always 
evaluate to 'true'}}
+       array[0]) {}
+
+   if (array ||  // expected-warning {{address of array 'array' will always 
evaluate to 'true'}}
+       (!array && array[0])) {} // expected-warning {{address of array 'array' 
will always evaluate to 'true'}}
+ }
+
+

- Fariborz

On Oct 23, 2014, at 5:25 PM, Richard Trieu <[email protected]> wrote:

On Thu, Oct 23, 2014 at 10:24 AM, jahanian <[email protected]> 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.

_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: patch for warning on logical negation with known result

Reply via email to