diff mbox

[C++] Enhancements to -Wconversion-null

Message ID AANLkTin88h6fTsaH6tUsx-vZlWMPxD17VarydBdAk9Sk@mail.gmail.com
State New
Headers show

Commit Message

Le-Chun Wu June 25, 2010, 11:29 p.m. UTC 
Hi,

This patch addresses two issues in the current implementation of
-Wconversion-null warning:

1. The current implementation warns on converting false to NULL for
function arguments, but not on other expressions/statements. That is,
it currently warns on

  void func1(int* ptr);

  void func2() {
    func1(false);
  }

but not on

  int* t = false;

This patch fixes this false-negative.

2. For templatized function calls, the current implementation will
warn even if a NULL passed in as a function parameter is used to
instantiate the template, which doesn't look right. For example, if we
have this template function:

  template<typename T> void func(T x) {}

we want to warn in this call as the user is clearly passing a NULL for
an integer parameter:

  void foo() {
    func<int>(NULL);
  }

but we don't want to warn in this call because the user clearly wants
to pass in a NULL:

  void foo() {
    func(NULL);
  }

However, the current implementation warns on both cases. Basically if
a function argument is NULL and is used to implicitly instantiate
a template function (and therefore bind the type of the corresponding
parameter to 'long int', which is the type of NULL in C++), we don't
want to warn about passing NULL to non-pointer argument. This patch
addresses this issue.

Bootstrapped and tested on x86_64-linux-gnu. OK for trunk?

Thanks,

Le-chun


2010-06-25  Le-Chun Wu  <lcwu@google.com>

gcc/cp/ChangeLog
        * call.c (build_new_function_call): Modify to track if a call has
        explicit template arguments.
        (conversion_null_warnings): Warn on converting false to NULL for
        non-function calls as well.
        (build_over_call): Do not warn on implicit instantiation with NULL.
        (build_new_method_call): Modify to track if a call has explicit
        template arguments.
        * cp-tree.h: Add a new flag LOOKUP_EXPLICIT_TMPL_ARGS.

gcc/testsuite/ChangeLog
        * g++.dg/warn/Wconversion-null-2.C: Change the test as we no long emit
        warnings on implicit instantiation with NULL.
        * g++.dg/warn/Wconversion-null-3.C: New test.
        * g++.dg/warn/Wconversion-null-4.C: New test.
        * g++.dg/warn/Wconversion-null.C: Change comments (no warning on
        implicit instantiation with NULL).
        * g++.old-deja/g++.other/null3.C: Change the test as we start to warn
        on converting false to NULL in assignment.

Comments

Jason Merrill June 30, 2010, 6:53 p.m. UTC | #1 
On 06/25/2010 07:29 PM, Le-Chun Wu wrote:
> +      if (arg == null_node
> +          && DECL_TEMPLATE_INFO (fn)
> +          && cand->template_decl
> +          && !(flags & LOOKUP_EXPLICIT_TMPL_ARGS))
> +        conversion_warning = false;

1) You don't need a new flag, you can just test cand->explicit_targs.

2) Testing for any explicit template arguments isn't really the right 
test: consider

template <class T> void f(T, int);

int main()
{
   f(1.0, NULL);
}

This call has no explicit template arguments, but it still passes NULL 
to a parameter that was not deduced from NULL.

3) Why do you want to suppress the warning in the deduced case, anyway? 
  Passing NULL to f(T) will still mean that we're calling f(uintptr_t) 
rather than f(some pointer), and is probably still wrong.

Jason
diff mbox

Patch

Index: gcc/cp/call.c
===================================================================
--- gcc/cp/call.c	(revision 161298)
+++ gcc/cp/call.c	(working copy)
@@ -3240,7 +3240,16 @@  build_new_function_call (tree fn, VEC(tr
       result = error_mark_node;
     }
   else
-    result = build_over_call (cand, LOOKUP_NORMAL, complain);
+    {
+      int flags = LOOKUP_NORMAL;
+      /* If fn is template_id_expr, the call has explicit template arguments
+         (e.g. func<int>(5)), communicate this info to build_over_call
+         through FLAGS so that later we can use it to decide whether to warn
+         about peculiar null pointer conversion.  */
+      if (TREE_CODE (fn) == TEMPLATE_ID_EXPR)
+        flags |= LOOKUP_EXPLICIT_TMPL_ARGS;
+      result = build_over_call (cand, flags, complain);
+    }

   /* Free all the conversions we allocated.  */
   obstack_free (&conversion_obstack, p);
@@ -4870,10 +4879,16 @@  conversion_null_warnings (tree totype, t
     }

   /* Issue warnings if "false" is converted to a NULL pointer */
-  else if (expr == boolean_false_node && fn && POINTER_TYPE_P (t))
-    warning_at (input_location, OPT_Wconversion_null,
-		"converting %<false%> to pointer type for argument %P of %qD",
-		argnum, fn);
+  else if (expr == boolean_false_node && POINTER_TYPE_P (t))
+    {
+      if (fn)
+        warning_at (input_location, OPT_Wconversion_null,
+                    "converting %<false%> to pointer type for
argument %P of %qD",
+                    argnum, fn);
+      else
+        warning_at (input_location, OPT_Wconversion_null,
+                    "converting %<false%> to pointer type");
+    }
 }

 /* Perform the conversions in CONVS on the expression EXPR.  FN and
@@ -5722,6 +5737,7 @@  build_over_call (struct z_candidate *can
     {
       tree type = TREE_VALUE (parm);
       tree arg = VEC_index (tree, args, arg_index);
+      bool conversion_warning = true;

       conv = convs[i];

@@ -5731,6 +5747,32 @@  build_over_call (struct z_candidate *can
 	  && !TREE_ADDRESSABLE (type))
 	conv = conv->u.next;

+      /* If the argument is NULL and used to (implicitly) instantiate a
+         template function (and bind one of the template arguments to
+         the type of 'long int'), we don't want to warn about passing NULL
+         to non-pointer argument.
+         For example, if we have this template function:
+
+           template<typename T> void func(T x) {}
+
+         we want to warn (with -Wconversion-null) in this case:
+
+           void foo() {
+             func<int>(NULL);
+           }
+
+         but not in this case:
+
+           void foo() {
+             func(NULL);
+           }
+      */
+      if (arg == null_node
+          && DECL_TEMPLATE_INFO (fn)
+          && cand->template_decl
+          && !(flags & LOOKUP_EXPLICIT_TMPL_ARGS))
+        conversion_warning = false;
+
       /* Warn about initializer_list deduction that isn't currently in the
 	 working draft.  */
       if (cxx_dialect > cxx98
@@ -5761,7 +5803,10 @@  build_over_call (struct z_candidate *can
 	    }
 	}

-      val = convert_like_with_context (conv, arg, fn, i-is_method, complain);
+      val = convert_like_with_context (conv, arg, fn, i-is_method,
+                                       conversion_warning
+                                       ? complain
+                                       : complain & (~tf_warning));

       val = convert_for_arg_passing (type, val);
       if (val == error_mark_node)
@@ -6501,6 +6546,12 @@  build_new_method_call (tree instance, tr
 	      if (DECL_VINDEX (fn) && ! (flags & LOOKUP_NONVIRTUAL)
 		  && resolves_to_fixed_type_p (instance, 0))
 		flags |= LOOKUP_NONVIRTUAL;
+              /* If this call has explicit template arguments, communicate
+                 this info to build_over_call through FLAGS so that later
+                 we can use it to decide whether to warn about peculiar null
+                 pointer conversion.  */
+              if (explicit_targs)
+                flags |= LOOKUP_EXPLICIT_TMPL_ARGS;
 	      /* Now we know what function is being called.  */
 	      if (fn_p)
 		*fn_p = fn;
Index: gcc/cp/cp-tree.h
===================================================================
--- gcc/cp/cp-tree.h	(revision 161298)
+++ gcc/cp/cp-tree.h	(working copy)
@@ -4168,6 +4168,8 @@  enum overload_flags { NO_SPECIAL = 0, DT
 #define LOOKUP_COPY_PARM (LOOKUP_NO_COPY_CTOR_CONVERSION << 1)
 /* We only want to consider list constructors.  */
 #define LOOKUP_LIST_ONLY (LOOKUP_COPY_PARM << 1)
+/* An instantiation with explicit template arguments.  */
+#define LOOKUP_EXPLICIT_TMPL_ARGS (LOOKUP_LIST_ONLY << 1)

 #define LOOKUP_NAMESPACES_ONLY(F)  \
   (((F) & LOOKUP_PREFER_NAMESPACES) && !((F) & LOOKUP_PREFER_TYPES))
Index: gcc/testsuite/g++.old-deja/g++.other/null3.C
===================================================================
--- gcc/testsuite/g++.old-deja/g++.other/null3.C	(revision 161298)
+++ gcc/testsuite/g++.old-deja/g++.other/null3.C	(working copy)
@@ -2,5 +2,5 @@ 

 void x()
 {
- int* p = 1==0;
+ int* p = 1==0; // { dg-warning "converting 'false' to pointer" }
 }
Index: gcc/testsuite/g++.dg/warn/Wconversion-null-2.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Wconversion-null-2.C	(revision 161298)
+++ gcc/testsuite/g++.dg/warn/Wconversion-null-2.C	(working copy)
@@ -44,6 +44,6 @@  int main()
   k(NULL);   // { dg-warning "" } converting NULL to int
   g(NULL);   // { dg-warning "" } converting NULL to int
   h<NULL>(); // No warning: NULL bound to integer template parameter
-  l(NULL);   // { dg-warning "" } converting NULL to int
+  l(NULL);   // No warning: NULL implicitly instantiated and bound to long
   NULL && NULL; // No warning: converting NULL to bool is OK
 }
Index: gcc/testsuite/g++.dg/warn/Wconversion-null-3.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Wconversion-null-3.C	(revision 0)
+++ gcc/testsuite/g++.dg/warn/Wconversion-null-3.C	(revision 0)
@@ -0,0 +1,15 @@ 
+// { dg-do compile }
+// { dg-options "" }
+
+#include <stddef.h>
+
+void func1(int* ptr);
+
+void func2() {
+  int* t = false;             // { dg-warning "converting 'false' to pointer" }
+  int* p;
+  p = false;                  // { dg-warning "converting 'false' to pointer" }
+  int* r = sizeof(char) / 2;
+  func1(false);               // { dg-warning "converting 'false' to pointer" }
+  int i = NULL;               // { dg-warning "converting to non-pointer" }
+}
Index: gcc/testsuite/g++.dg/warn/Wconversion-null-4.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Wconversion-null-4.C	(revision 0)
+++ gcc/testsuite/g++.dg/warn/Wconversion-null-4.C	(revision 0)
@@ -0,0 +1,45 @@ 
+// { dg-do compile }
+// { dg-options "" }
+
+#include <stddef.h>
+
+class Foo {
+ public:
+  template <typename T1, typename T2>
+  static void Compare(const T1& expected, const T2& actual) { }
+
+  template <typename T1, typename T2>
+  static void Compare(const T1& expected, T2* actual) { }
+
+};
+
+template<typename T1>
+class Foo2 {
+ public:
+  Foo2(int x);
+  template<typename T2> void Bar(T2 y);
+};
+
+template<typename T3> void func(T3 x) { }
+
+typedef Foo2<int> MyFooType;
+
+void func1(long int a) {
+  MyFooType *foo2 = new MyFooType(NULL); // { dg-warning "passing NULL to" }
+  foo2->Bar(a);
+  func(NULL);                            // Shoud not warn
+  func<int>(NULL);                       // { dg-warning "passing NULL to" }
+  func<int *>(NULL);
+}
+
+int x = 1;
+
+main()
+{
+  int *p = &x;
+
+  Foo::Compare(0, *p);
+  Foo::Compare<long int, int>(NULL, p);  // { dg-warning "passing NULL to" }
+  Foo::Compare(NULL, p);                 // should not warn
+  func1(NULL);                           // { dg-warning "passing NULL to" }
+}
Index: gcc/testsuite/g++.dg/warn/Wconversion-null.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Wconversion-null.C	(revision 161298)
+++ gcc/testsuite/g++.dg/warn/Wconversion-null.C	(working copy)
@@ -44,6 +44,6 @@  int main()
   k(NULL);   //  converting NULL to int
   g(NULL);   //  converting NULL to int
   h<NULL>(); // No warning: NULL bound to integer template parameter
-  l(NULL);   //  converting NULL to int
+  l(NULL);   // No warning: NULL implicitly instantiated and bound to long
   NULL && NULL; // No warning: converting NULL to bool is OK
 }