diff mbox

[C++,preview] PR 44277

Message ID 4EAD9AE8.7070703@oracle.com
State New
Headers show

Commit Message

Paolo Carlini Oct. 30, 2011, 6:43 p.m. UTC 
Hi,

thus I'm working on a new warning for 0 as null ptr constant, which 
should be useful for people moving to C++11' nullptr. Looks like a good 
place to check for that is cp_convert_to_pointer, then the difficulty is 
coping correctly with things like:

     int* p;

     if (p)
       ;

where certainly we don't want to warn, but the front-end would, 
normally, because the latter is implicitly transformed via 
cp_truthvalue_conversion to

     if (p != 0)
       ;

Thus my idea: stop generating those implicit zeros and generate instead, 
internally, exactly nullptrs when pointers are involved! The idea 
appears to work pretty well, the substance of it is in very few lines in 
cp/typeck.c and cp/init.c, in particular in cp_truthvalue_conversion itself.

However, there is some complexity in the actual implementation, because 
we forward to the shared c_common_truthvalue_conversion, which does some 
non-trivial simplifications, and of course doesn't know about 
nullptr_node. To solve this I'm adding a parameter to it and passing 
nullptr_node or integer_zero_node, when appropriate. If unsharing 
c_common_truthvalue_conversion isn't an option , definitely would add a 
lot of redundant code, I don't see a neater way to accomplish the same 
result. Well, unless we could move nullptr_node from cp/ only to the 
global tree?!? In this case the patch would become tiny.

Thanks for any feedback!

(patch below passes the testsuite and does the right thing for the basic 
testcase)

Paolo.

/////////////////////

Comments

Jason Merrill Oct. 31, 2011, 2:32 p.m. UTC | #1 
How does it work to warn in convert_like_real instead?

Jason
Paolo Carlini Oct. 31, 2011, 4:51 p.m. UTC | #2 
Hi,
> How does it work to warn in convert_like_real instead?
the problem is that (expr, totype) can be a lot of different things for 
which we want to warn, can be a zero and a pointer for assignments, but, 
when totype is a BOOLEAN_TYPE expr can be an EQ_EXPR or NEQ_EXPR and 
then the operands various things depending on whether we are looking for 
pointer, data member pointer, etc, on the left or on the right of the == 
or != sign. In other terms, the pattern matching doesn't seem matter of 
a few lines. I'm annoyed by this. Also, for the assignment case, I'm 
getting duplicate warnings, maybe can be fixed.

Do you think there is no neat way to implement my idea of avoiding 
generating those implicit 0s in the first place? The internals of the 
front-end, seem very c++98-ish for null pointers ;)

Paolo.
diff mbox

Patch

Index: c-family/c.opt
===================================================================
--- c-family/c.opt	(revision 180671)
+++ c-family/c.opt	(working copy)
@@ -685,6 +685,9 @@  Wpointer-sign
 C ObjC Var(warn_pointer_sign) Init(-1) Warning
 Warn when a pointer differs in signedness in an assignment
 
+Wzero-as-null-pointer-constant
+C++ ObjC++ Var(warn_zero_as_null_pointer_constant) Warning
+
 ansi
 C ObjC C++ ObjC++
 A synonym for -std=c89 (for C) or -std=c++98 (for C++)
Index: c-family/c-common.c
===================================================================
--- c-family/c-common.c	(revision 180671)
+++ c-family/c-common.c	(working copy)
@@ -3898,7 +3898,7 @@  decl_with_nonnull_addr_p (const_tree expr)
    The resulting type should always be `truthvalue_type_node'.  */
 
 tree
-c_common_truthvalue_conversion (location_t location, tree expr)
+c_common_truthvalue_conversion (location_t location, tree expr, tree zeronode)
 {
   switch (TREE_CODE (expr))
     {
@@ -3921,9 +3921,11 @@  tree
 	return expr;
       expr = build2 (TREE_CODE (expr), truthvalue_type_node,
 		     c_common_truthvalue_conversion (location,
-						     TREE_OPERAND (expr, 0)),
+						     TREE_OPERAND (expr, 0),
+						     zeronode),
 		     c_common_truthvalue_conversion (location,
-						     TREE_OPERAND (expr, 1)));
+						     TREE_OPERAND (expr, 1),
+						     zeronode));
       goto ret;
 
     case TRUTH_NOT_EXPR:
@@ -3931,7 +3933,8 @@  tree
 	return expr;
       expr = build1 (TREE_CODE (expr), truthvalue_type_node,
 		     c_common_truthvalue_conversion (location,
-						     TREE_OPERAND (expr, 0)));
+						     TREE_OPERAND (expr, 0),
+						     zeronode));
       goto ret;
 
     case ERROR_MARK:
@@ -3976,9 +3979,11 @@  tree
 			      (TREE_SIDE_EFFECTS (TREE_OPERAND (expr, 1))
 			       ? TRUTH_OR_EXPR : TRUTH_ORIF_EXPR),
 		c_common_truthvalue_conversion (location,
-						TREE_OPERAND (expr, 0)),
+						TREE_OPERAND (expr, 0),
+						zeronode),
 		c_common_truthvalue_conversion (location,
-						TREE_OPERAND (expr, 1)),
+						TREE_OPERAND (expr, 1),
+						zeronode),
 			      0);
       goto ret;
 
@@ -3987,7 +3992,8 @@  tree
     case FLOAT_EXPR:
     case EXCESS_PRECISION_EXPR:
       /* These don't change whether an object is nonzero or zero.  */
-      return c_common_truthvalue_conversion (location, TREE_OPERAND (expr, 0));
+      return c_common_truthvalue_conversion (location, TREE_OPERAND (expr, 0),
+					     zeronode);
 
     case LROTATE_EXPR:
     case RROTATE_EXPR:
@@ -3998,12 +4004,13 @@  tree
 	  expr = build2 (COMPOUND_EXPR, truthvalue_type_node,
 			 TREE_OPERAND (expr, 1),
 			 c_common_truthvalue_conversion
-			 (location, TREE_OPERAND (expr, 0)));
+			 (location, TREE_OPERAND (expr, 0), zeronode));
 	  goto ret;
 	}
       else
 	return c_common_truthvalue_conversion (location,
-					       TREE_OPERAND (expr, 0));
+					       TREE_OPERAND (expr, 0),
+					       zeronode);
 
     case COND_EXPR:
       /* Distribute the conversion into the arms of a COND_EXPR.  */
@@ -4013,9 +4020,9 @@  tree
 	  tree op2 = TREE_OPERAND (expr, 2);
 	  /* In C++ one of the arms might have void type if it is throw.  */
 	  if (!VOID_TYPE_P (TREE_TYPE (op1)))
-	    op1 = c_common_truthvalue_conversion (location, op1);
+	    op1 = c_common_truthvalue_conversion (location, op1, zeronode);
 	  if (!VOID_TYPE_P (TREE_TYPE (op2)))
-	    op2 = c_common_truthvalue_conversion (location, op2);
+	    op2 = c_common_truthvalue_conversion (location, op2, zeronode);
 	  expr = fold_build3_loc (location, COND_EXPR, truthvalue_type_node,
 				  TREE_OPERAND (expr, 0), op1, op2);
 	  goto ret;
@@ -4026,9 +4033,11 @@  tree
 	  expr = build3 (COND_EXPR, truthvalue_type_node,
 			 TREE_OPERAND (expr, 0),
 			 c_common_truthvalue_conversion (location,
-							 TREE_OPERAND (expr, 1)),
+							 TREE_OPERAND (expr, 1),
+							 zeronode),
 			 c_common_truthvalue_conversion (location,
-							 TREE_OPERAND (expr, 2)));
+							 TREE_OPERAND (expr, 2),
+							 zeronode));
 	  goto ret;
 	}
 
@@ -4050,7 +4059,8 @@  tree
 	/* If this isn't narrowing the argument, we can ignore it.  */
 	if (TYPE_PRECISION (totype) >= TYPE_PRECISION (fromtype))
 	  return c_common_truthvalue_conversion (location,
-						 TREE_OPERAND (expr, 0));
+						 TREE_OPERAND (expr, 0),
+						 zeronode);
       }
       break;
 
@@ -4077,10 +4087,10 @@  tree
 		? TRUTH_OR_EXPR : TRUTH_ORIF_EXPR),
 	c_common_truthvalue_conversion
 	       (location,
-		build_unary_op (location, REALPART_EXPR, t, 0)),
+		build_unary_op (location, REALPART_EXPR, t, 0), zeronode),
 	c_common_truthvalue_conversion
 	       (location,
-		build_unary_op (location, IMAGPART_EXPR, t, 0)),
+		build_unary_op (location, IMAGPART_EXPR, t, 0), zeronode),
 	       0));
       goto ret;
     }
@@ -4093,7 +4103,7 @@  tree
       return build_binary_op (location, NE_EXPR, expr, fixed_zero_node, 1);
     }
   else
-    return build_binary_op (location, NE_EXPR, expr, integer_zero_node, 1);
+    return build_binary_op (location, NE_EXPR, expr, zeronode, 1);
 
  ret:
   protected_set_expr_location (expr, location);
Index: c-family/c-common.h
===================================================================
--- c-family/c-common.h	(revision 180671)
+++ c-family/c-common.h	(working copy)
@@ -753,7 +753,7 @@  extern tree c_fully_fold (tree, bool, bool *);
 extern tree decl_constant_value_for_optimization (tree);
 extern tree c_wrap_maybe_const (tree, bool);
 extern tree c_save_expr (tree);
-extern tree c_common_truthvalue_conversion (location_t, tree);
+extern tree c_common_truthvalue_conversion (location_t, tree, tree);
 extern void c_apply_type_quals_to_decl (int, tree);
 extern tree c_sizeof_or_alignof_type (location_t, tree, bool, int);
 extern tree c_alignof_expr (location_t, tree);
Index: cp/typeck.c
===================================================================
--- cp/typeck.c	(revision 180665)
+++ cp/typeck.c	(working copy)
@@ -4058,7 +4058,9 @@  cp_build_binary_op (location_t location,
      	  else 
 	    {
 	      op0 = build_ptrmemfunc_access_expr (op0, pfn_identifier);
-	      op1 = cp_convert (TREE_TYPE (op0), integer_zero_node); 
+	      op1 = cp_convert (TREE_TYPE (op0),
+				NULLPTR_TYPE_P (TREE_TYPE (op1))
+				? nullptr_node : integer_zero_node);
 	    }
 	  result_type = TREE_TYPE (op0);
 	}
@@ -4668,9 +4670,12 @@  cp_truthvalue_conversion (tree expr)
   tree type = TREE_TYPE (expr);
   if (TYPE_PTRMEM_P (type))
     return build_binary_op (EXPR_LOCATION (expr),
-			    NE_EXPR, expr, integer_zero_node, 1);
+			    NE_EXPR, expr, nullptr_node, 1);
   else
-    return c_common_truthvalue_conversion (input_location, expr);
+    return c_common_truthvalue_conversion (input_location, expr,
+					   (TYPE_PTR_P (type)
+					    || TYPE_PTRMEMFUNC_P (type))
+					   ? nullptr_node : integer_zero_node);
 }
 
 /* Just like cp_truthvalue_conversion, but we want a CLEANUP_POINT_EXPR.  */
Index: cp/init.c
===================================================================
--- cp/init.c	(revision 180665)
+++ cp/init.c	(working copy)
@@ -176,6 +176,8 @@  build_zero_init_1 (tree type, tree nelts, bool sta
        items with static storage duration that are not otherwise
        initialized are initialized to zero.  */
     ;
+  else if (TYPE_PTR_P (type) || TYPE_PTR_TO_MEMBER_P (type))
+    init = convert (type, nullptr_node);
   else if (SCALAR_TYPE_P (type))
     init = convert (type, integer_zero_node);
   else if (CLASS_TYPE_P (type))
@@ -1112,8 +1114,10 @@  expand_cleanup_for_base (tree binfo, tree flag)
   if (flag)
     expr = fold_build3_loc (input_location,
 			COND_EXPR, void_type_node,
-			c_common_truthvalue_conversion (input_location, flag),
-			expr, integer_zero_node);
+			    c_common_truthvalue_conversion (input_location,
+							    flag,
+							    integer_zero_node),
+			    expr, integer_zero_node);
 
   finish_eh_cleanup (expr);
 }
Index: cp/rtti.c
===================================================================
--- cp/rtti.c	(revision 180665)
+++ cp/rtti.c	(working copy)
@@ -747,7 +747,8 @@  build_dynamic_cast_1 (tree type, tree expr, tsubst
 	      tree neq;
 
 	      result = save_expr (result);
-	      neq = c_common_truthvalue_conversion (input_location, result);
+	      neq = c_common_truthvalue_conversion (input_location, result,
+						    integer_zero_node);
 	      return cp_convert (type,
 				 build3 (COND_EXPR, TREE_TYPE (result),
 					 neq, result, bad));
Index: cp/cvt.c
===================================================================
--- cp/cvt.c	(revision 180665)
+++ cp/cvt.c	(working copy)
@@ -198,6 +198,10 @@  cp_convert_to_pointer (tree type, tree expr)
 
   if (null_ptr_cst_p (expr))
     {
+      if (!NULLPTR_TYPE_P (TREE_TYPE (expr)))
+	warning (OPT_Wzero_as_null_pointer_constant,
+		 "zero as null pointer constant");
+
       if (TYPE_PTRMEMFUNC_P (type))
 	return build_ptrmemfunc (TYPE_PTRMEMFUNC_FN_TYPE (type), expr, 0,
 				 /*c_cast_p=*/false, tf_warning_or_error);
Index: c-typeck.c
===================================================================
--- c-typeck.c	(revision 180665)
+++ c-typeck.c	(working copy)
@@ -9869,8 +9869,10 @@  build_binary_op (location_t location, enum tree_co
 	     but that does not mean the operands should be
 	     converted to ints!  */
 	  result_type = integer_type_node;
-	  op0 = c_common_truthvalue_conversion (location, op0);
-	  op1 = c_common_truthvalue_conversion (location, op1);
+	  op0 = c_common_truthvalue_conversion (location, op0,
+						integer_zero_node);
+	  op1 = c_common_truthvalue_conversion (location, op1,
+						integer_zero_node);
 	  converted = 1;
 	  boolean_op = true;
 	}
@@ -10587,7 +10589,7 @@  c_objc_common_truthvalue_conversion (location_t lo
 
   /* ??? Should we also give an error for vectors rather than leaving
      those to give errors later?  */
-  expr = c_common_truthvalue_conversion (location, expr);
+  expr = c_common_truthvalue_conversion (location, expr, integer_zero_node);
 
   if (TREE_CODE (expr) == INTEGER_CST && int_operands && !int_const)
     {