diff mbox

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

Message ID CAH6eHdRE9192yx6QU1GEQviQ5iSz5zUkNuaH8KJcoojmUDZnDQ@mail.gmail.com
State New
Headers show

Commit Message

Jonathan Wakely Nov. 7, 2011, 9:43 p.m. UTC 
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.

Comments

Jason Merrill Nov. 7, 2011, 9:47 p.m. UTC | #1 
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
Gabriel Dos Reis Nov. 7, 2011, 10:38 p.m. UTC | #2 
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.
>
Jason Merrill Nov. 7, 2011, 10:39 p.m. UTC | #3 
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
Jonathan Wakely Nov. 7, 2011, 11:03 p.m. UTC | #4 
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.
Gabriel Dos Reis Nov. 7, 2011, 11:48 p.m. UTC | #5 
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
diff mbox

Patch

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