[patch] c++/2972 warn when ctor-initializer leaves uninitialized data

[patch] c++/2972 warn when ctor-initializer leaves uninitialized data

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 1097 bytes --]

This is a new version of my -Wmeminit patch, first posted to PR c++/2972.

Jason suggested combining the Wmeminit warning with the adjacent
Weffc++ one which I agree with.  The advice in the Effective C++ book
actually says not to leave members uninitialized, rather than saying
*all* members must have a mem-initializer (which is just annoying in
many cases if the type has a safe default constructor), and my patch
provides a better check for that by only warning when data is left
uninitialized.

Unfortunately this doesn't work very well in C++11 mode, as defaulted
constructors don't cause warnings when they should do e.g.

struct C
{
  int i;
  C() = default;
};

This doesn't produce the same warning as C() {} even though that's
what the defaulted constructor is equivalent to.

I'm posting it for comment and in case anyone else has time to work on it.

        * c-family/c.opt (Wmeminit): Add new option.
        * c-family/c-opts.c: Include it in Wecpp
        * cp/init.c: Implement Wmeminit.
        * doc/invoke.texi: document it.

        * testsuite/g++.dg/warn/Wmeminit.C: New.

[-- Attachment #2: 2972.patch --]
[-- Type: text/x-patch, Size: 4608 bytes --]

Index: c-family/c.opt
===================================================================
--- c-family/c.opt	(revision 181096)
+++ c-family/c.opt	(working copy)
@@ -461,6 +461,10 @@
 C ObjC C++ ObjC++ Var(warn_main) Init(-1) Warning
 Warn about suspicious declarations of \"main\"
 
+Wmeminit
+C++ Var(warn_meminit) Warning
+Warn about missing member initializers in constructors which leave data uninitialized
+
 Wmissing-braces
 C ObjC C++ ObjC++ Var(warn_missing_braces) Warning
 Warn about possibly missing braces around initializers
Index: c-family/c-opts.c
===================================================================
--- c-family/c-opts.c	(revision 181096)
+++ c-family/c-opts.c	(working copy)
@@ -550,7 +550,14 @@
     case OPT_Weffc__:
       warn_ecpp = value;
       if (value)
-        warn_nonvdtor = true;
+        {
+          /* Effective C++ rule 12 says to prefer using a mem-initializer
+             to assignment. */
+          warn_meminit = true;
+          /* Effective C++ rule 14 says to declare destructors virtual
+             in polymorphic classes. */
+          warn_nonvdtor = true;
+        }
       break;
 
     case OPT_ansi:
Index: cp/init.c
===================================================================
--- cp/init.c	(revision 181096)
+++ cp/init.c	(working copy)
@@ -518,13 +518,27 @@
 	}
     }
 
-  /* Effective C++ rule 12 requires that all data members be
-     initialized.  */
-  if (warn_ecpp && init == NULL_TREE && TREE_CODE (type) != ARRAY_TYPE)
-    warning_at (DECL_SOURCE_LOCATION (current_function_decl), OPT_Weffc__,
-		"%qD should be initialized in the member initialization list",
-		member);
+  /* Warn if there is no initializer for a member which will leave data
+     uninitialized. */
 
+  if (warn_meminit && init == NULL_TREE)
+    {
+      tree field = default_init_uninitialized_part (type);
+      if (field)
+        {
+          if (DECL_P (field))
+            warning_at (DECL_SOURCE_LOCATION (current_function_decl),
+                        OPT_Wmeminit,
+                        "%qD is not initialized in the member initialization"
+                        " list, so %q+#D is uninitialized", member, field);
+          else
+            warning_at (DECL_SOURCE_LOCATION (current_function_decl),
+                        OPT_Wmeminit,
+                        "%qD is not initialized in the member initialization"
+                        " list", member);
+        }
+    }
+
   /* Get an lvalue for the data member.  */
   decl = build_class_member_access_expr (current_class_ref, member,
 					 /*access_path=*/NULL_TREE,
Index: testsuite/g++.dg/warn/Wmeminit.C
===================================================================
--- testsuite/g++.dg/warn/Wmeminit.C	(revision 0)
+++ testsuite/g++.dg/warn/Wmeminit.C	(revision 0)
@@ -0,0 +1,72 @@
+// PR c++/2972
+// { dg-do compile }
+// { dg-options "-Wmeminit" }
+
+// Warn when a constructor (user-declared or implicitly-declared)
+// leaves members uninitialized.
+
+struct A 
+{
+  int i;
+  A() : i() { } // { dg-bogus "uninitialized" }
+};
+
+struct B 
+{
+  int i;
+  B() { } // { dg-warning "'B::i' is not initialized" }
+};
+
+struct C // { dg-bogus "uninitialized" }
+{
+  int i;
+}; 
+
+struct D
+{
+  C c;
+  D() : c() { } // { dg-bogus "uninitialized" }
+};
+
+struct E
+{
+  int i; // { dg-warning "'F::e' is not initialized" }
+}; 
+
+struct F
+{
+  E e;
+  F() { }
+};
+
+struct G
+{
+  int i; // { dg-warning "'H::g' is not initialized" }
+}; 
+
+struct H
+{
+  G g;
+  H(const H&) { }
+};
+
+struct I // { dg-warning "'I::i' is not initialized" }
+{
+  int i;
+};
+
+struct J : I
+{
+  J() { }
+};
+
+struct K // { dg-warning "'K::i' is not initialized" }
+{
+  int i;
+};
+
+struct L : K
+{
+  L(const L&) { }
+};
+
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 181096)
+++ doc/invoke.texi	(working copy)
@@ -2371,6 +2371,22 @@
 base class does not have a virtual destructor.  This warning is enabled
 by @option{-Wall}.
 
+@item -Wmeminit @r{(C++ and Objective-C++ only)}
+@opindex Wmeminit
+@opindex Wno-meminit
+Warn about missing member initializers in constructors which leave data
+uninitialized. A warning will still be given even if the member is
+initialized in the constructor body e.g.
+
+@smallexample
+struct A @{
+  int i;
+  A() @{ i = 0; @} // warning: 'A::i' is not initialized
+@};
+@end smallexample
+
+This warning is enabled if -Weffc++ is specified.
+
 @item -Wnarrowing @r{(C++ and Objective-C++ only)}
 @opindex Wnarrowing
 @opindex Wno-narrowing

Re: [patch] c++/2972 warn when ctor-initializer leaves uninitialized data

[Jason Merrill]

On 11/07/2011 04:43 PM, Jonathan Wakely wrote:
> Unfortunately this doesn't work very well in C++11 mode, as defaulted
> constructors don't cause warnings when they should do e.g.

Maybe check this in defaulted_late_check?

Jason

Re: [patch] c++/2972 warn when ctor-initializer leaves uninitialized data

[Gabriel Dos Reis]

On Mon, Nov 7, 2011 at 3:43 PM, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> This is a new version of my -Wmeminit patch, first posted to PR c++/2972.
>
> Jason suggested combining the Wmeminit warning with the adjacent
> Weffc++ one which I agree with.  The advice in the Effective C++ book
> actually says not to leave members uninitialized, rather than saying
> *all* members must have a mem-initializer (which is just annoying in
> many cases if the type has a safe default constructor), and my patch
> provides a better check for that by only warning when data is left
> uninitialized.
>
> Unfortunately this doesn't work very well in C++11 mode, as defaulted
> constructors don't cause warnings when they should do e.g.
>
> struct C
> {
>  int i;
>  C() = default;
> };
>
> This doesn't produce the same warning as C() {} even though that's
> what the defaulted constructor is equivalent to.

so the defaulted constructor does not initialize C::i?

>
> I'm posting it for comment and in case anyone else has time to work on it.
>
>        * c-family/c.opt (Wmeminit): Add new option.
>        * c-family/c-opts.c: Include it in Wecpp
>        * cp/init.c: Implement Wmeminit.
>        * doc/invoke.texi: document it.
>
>        * testsuite/g++.dg/warn/Wmeminit.C: New.
>

Re: [patch] c++/2972 warn when ctor-initializer leaves uninitialized data

[Jason Merrill]

On 11/07/2011 05:38 PM, Gabriel Dos Reis wrote:
>> struct C
>> {
>>   int i;
>>   C() = default;
>> };
>
> so the defaulted constructor does not initialize C::i?

No, it doesn't.  value-initialization of a C will initialize it, but not 
default-initialization.

Jason

Re: [patch] c++/2972 warn when ctor-initializer leaves uninitialized data

[Jonathan Wakely]

On 7 November 2011 22:38, Gabriel Dos Reis wrote:
>> Unfortunately this doesn't work very well in C++11 mode, as defaulted
>> constructors don't cause warnings when they should do e.g.
>>
>> struct C
>> {
>>  int i;
>>  C() = default;
>> };
>>
>> This doesn't produce the same warning as C() {} even though that's
>> what the defaulted constructor is equivalent to.
>
> so the defaulted constructor does not initialize C::i?

[class.ctor] p6

A default constructor that is defaulted and not defined as deleted is
implicitly defined when it is odr-used (3.2) to create an object of its
class type (1.8) or when it is explicitly defaulted after its first
declaration. The implicitly-defined default constructor performs the
set of initializations of the class that would be
performed by a user-written default constructor for that class with no
ctor-initializer (12.6.2) and an empty compound-statement.

Re: [patch] c++/2972 warn when ctor-initializer leaves uninitialized data

[Gabriel Dos Reis]

On Mon, Nov 7, 2011 at 4:39 PM, Jason Merrill <jason@redhat.com> wrote:
> On 11/07/2011 05:38 PM, Gabriel Dos Reis wrote:
>>>
>>> struct C
>>> {
>>>  int i;
>>>  C() = default;
>>> };
>>
>> so the defaulted constructor does not initialize C::i?
>
> No, it doesn't.  value-initialization of a C will initialize it, but not
> default-initialization.
>

Thanks! (obvously I was stuck with earlier variation of the proposal.)

-- Gaby

Re: [patch] c++/2972 warn when ctor-initializer leaves uninitialized data

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 423 bytes --]

On 7 November 2011 21:47, Jason Merrill wrote:
> On 11/07/2011 04:43 PM, Jonathan Wakely wrote:
>>
>> Unfortunately this doesn't work very well in C++11 mode, as defaulted
>> constructors don't cause warnings when they should do e.g.
>
> Maybe check this in defaulted_late_check?

I tried that (attached) and it does cause warnings in defaulted
constructed, but even for members with an NSDMI, which are not
uninitialized.

[-- Attachment #2: 2972.patch --]
[-- Type: text/x-patch, Size: 4719 bytes --]

Index: c-family/c.opt
===================================================================
--- c-family/c.opt	(revision 181173)
+++ c-family/c.opt	(working copy)
@@ -461,6 +461,10 @@ Wmain
 C ObjC C++ ObjC++ Var(warn_main) Init(-1) Warning
 Warn about suspicious declarations of \"main\"
 
+Wmeminit
+C++ Var(warn_meminit) Warning
+Warn about POD members which are not initialized in a constructor initialization list
+
 Wmissing-braces
 C ObjC C++ ObjC++ Var(warn_missing_braces) Warning
 Warn about possibly missing braces around initializers
Index: c-family/c-opts.c
===================================================================
--- c-family/c-opts.c	(revision 181173)
+++ c-family/c-opts.c	(working copy)
@@ -550,7 +550,14 @@ c_common_handle_option (size_t scode, co
     case OPT_Weffc__:
       warn_ecpp = value;
       if (value)
-        warn_nonvdtor = true;
+        {
+          /* Effective C++ rule 12 says to prefer using a mem-initializer
+             to assignment. */
+          warn_meminit = true;
+          /* Effective C++ rule 14 says to declare destructors virtual
+             in polymorphic classes. */
+          warn_nonvdtor = true;
+        }
       break;
 
     case OPT_ansi:
Index: cp/init.c
===================================================================
--- cp/init.c	(revision 181173)
+++ cp/init.c	(working copy)
@@ -485,6 +485,42 @@ build_value_init_noctor (tree type, tsub
   return build_zero_init (type, NULL_TREE, /*static_storage_p=*/false);
 }
 
+/* Warn if default initialization of MEMBER of type TYPE in constructor
+ * CONS will leave some parts uninitialized.  */
+
+static void
+warn_meminit_leaves_uninitialized (tree member, tree type, tree cons)
+{
+  tree field = default_init_uninitialized_part (type);
+  if (!field)
+    return;
+
+  if (DECL_P (field))
+    warning_at (DECL_SOURCE_LOCATION (cons), OPT_Wmeminit,
+                "no member initializer for %qD so %q+#D is uninitialized",
+                member, field);
+  else
+    warning_at (DECL_SOURCE_LOCATION (cons), OPT_Wmeminit,
+                "no member initializer for %qD so it is uninitialized",
+                member);
+}
+
+/* Warn if defaulted constructor CONS for TYPE with no mem-initializer-list
+   will leave uninitialized data.  */
+
+void
+warn_missing_meminits (tree type, tree cons)
+{
+  tree mem_inits = sort_mem_initializers (type, NULL_TREE);
+  while (mem_inits)
+    {
+      tree member = TREE_PURPOSE (mem_inits);
+      /* TODO do not warn if brace-or-equal-initializer */
+      warn_meminit_leaves_uninitialized (member, TREE_TYPE (member), cons);
+      mem_inits = TREE_CHAIN (mem_inits);
+    }
+}
+
 /* Initialize MEMBER, a FIELD_DECL, with INIT, a TREE_LIST of
    arguments.  If TREE_LIST is void_type_node, an empty initializer
    list was given; if NULL_TREE no initializer was given.  */
@@ -518,12 +554,10 @@ perform_member_init (tree member, tree i
 	}
     }
 
-  /* Effective C++ rule 12 requires that all data members be
-     initialized.  */
-  if (warn_ecpp && init == NULL_TREE && TREE_CODE (type) != ARRAY_TYPE)
-    warning_at (DECL_SOURCE_LOCATION (current_function_decl), OPT_Weffc__,
-		"%qD should be initialized in the member initialization list",
-		member);
+  /* Warn if there is no initializer for a member which will be left
+     uninitialized.  */
+  if (warn_meminit && init == NULL_TREE)
+      warn_meminit_leaves_uninitialized (member, type, current_function_decl);
 
   /* Get an lvalue for the data member.  */
   decl = build_class_member_access_expr (current_class_ref, member,
Index: cp/method.c
===================================================================
--- cp/method.c	(revision 181173)
+++ cp/method.c	(working copy)
@@ -1655,6 +1655,10 @@ defaulted_late_check (tree fn)
 
   if (DECL_DELETED_FN (implicit_fn))
     DECL_DELETED_FN (fn) = 1;
+
+  if (warn_meminit && (kind == sfk_constructor || kind == sfk_copy_constructor
+        || kind == sfk_move_constructor))
+    warn_missing_meminits (current_class_type, fn);
 }
 
 /* Returns true iff FN can be explicitly defaulted, and gives any
Index: cp/cp-tree.h
===================================================================
--- cp/cp-tree.h	(revision 181173)
+++ cp/cp-tree.h	(working copy)
@@ -4915,6 +4915,7 @@ extern bool user_provided_p			(tree);
 extern bool type_has_user_provided_constructor  (tree);
 extern bool type_has_user_provided_default_constructor (tree);
 extern tree default_init_uninitialized_part (tree);
+extern void warn_missing_meminits (tree, tree);
 extern bool trivial_default_constructor_is_constexpr (tree);
 extern bool type_has_constexpr_default_constructor (tree);
 extern bool type_has_virtual_destructor		(tree);

Re: [patch] c++/2972 warn when ctor-initializer leaves uninitialized data

[Jason Merrill]

On 11/10/2011 02:48 PM, Jonathan Wakely wrote:
> +warn_missing_meminits (tree type, tree cons)
> +{
> +  tree mem_inits = sort_mem_initializers (type, NULL_TREE);
> +  while (mem_inits)
> +    {
> +      tree member = TREE_PURPOSE (mem_inits);
> +      /* TODO do not warn if brace-or-equal-initializer */
> +      warn_meminit_leaves_uninitialized (member, TREE_TYPE (member), cons);
> +      mem_inits = TREE_CHAIN (mem_inits);
> +    }
> +}

Check DECL_INITIAL (member) to tell if it has an NSDMI.

Jason

Re: [patch] c++/2972 warn when ctor-initializer leaves uninitialized data

[Jason Merrill]

On 11/10/2011 03:10 PM, Jason Merrill wrote:
> On 11/10/2011 02:48 PM, Jonathan Wakely wrote:
>> +warn_missing_meminits (tree type, tree cons)
>> +{
>> + tree mem_inits = sort_mem_initializers (type, NULL_TREE);
>> + while (mem_inits)
>> + {
>> + tree member = TREE_PURPOSE (mem_inits);
>> + /* TODO do not warn if brace-or-equal-initializer */
>> + warn_meminit_leaves_uninitialized (member, TREE_TYPE (member), cons);
>> + mem_inits = TREE_CHAIN (mem_inits);
>> + }
>> +}
>
> Check DECL_INITIAL (member) to tell if it has an NSDMI.

Actually, why not just use default_init_uninitialized_part (type)?

> +  if (warn_meminit && (kind == sfk_constructor || kind == sfk_copy_constructor
> +        || kind == sfk_move_constructor))
> +    warn_missing_meminits (current_class_type, fn);

We only want to do this for sfk_constructor; the others initialize all 
fields.

Jason

Re: [patch] c++/2972 warn when ctor-initializer leaves uninitialized data

[Jonathan Wakely]

On 10 November 2011 20:17, Jason Merrill wrote:
> On 11/10/2011 03:10 PM, Jason Merrill wrote:
>>
>> On 11/10/2011 02:48 PM, Jonathan Wakely wrote:
>>>
>>> +warn_missing_meminits (tree type, tree cons)
>>> +{
>>> + tree mem_inits = sort_mem_initializers (type, NULL_TREE);
>>> + while (mem_inits)
>>> + {
>>> + tree member = TREE_PURPOSE (mem_inits);
>>> + /* TODO do not warn if brace-or-equal-initializer */
>>> + warn_meminit_leaves_uninitialized (member, TREE_TYPE (member), cons);
>>> + mem_inits = TREE_CHAIN (mem_inits);
>>> + }
>>> +}
>>
>> Check DECL_INITIAL (member) to tell if it has an NSDMI.
>
> Actually, why not just use default_init_uninitialized_part (type)?
>
>> +  if (warn_meminit && (kind == sfk_constructor || kind ==
>> sfk_copy_constructor
>> +        || kind == sfk_move_constructor))
>> +    warn_missing_meminits (current_class_type, fn);
>
> We only want to do this for sfk_constructor; the others initialize all
> fields.

Doh, of course.  Thanks for the pointers, I'll have another stab at
it. I really want to get this warning implemented eventually.