Bug 18016 - Warn about member variables initialized with itself
Warn about member variables initialized with itself
Status: RESOLVED FIXED
Product: gcc
Classification: Unclassified
Component: c++
3.4.2
: P2 enhancement
: 4.7.0
Assigned To: Not yet assigned to anyone
: diagnostic, patch
: 51533 55318 (view as bug list)
Depends on: 34772
Blocks: Wuninitialized
  Show dependency treegraph
 
Reported: 2004-10-15 16:44 UTC by ejb
Modified: 2012-11-14 11:40 UTC (History)
7 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2005-04-28 00:43:26


Attachments
source file that reproduces problem (252 bytes, text/plain)
2004-10-15 16:46 UTC, ejb
Details

Note You need to log in before you can comment on or make changes to this bug.
Description ejb 2004-10-15 16:44:57 UTC
First, my gcc version information:

% gcc -v
Reading specs from /usr/lib/gcc/i486-linux/3.4.2/specs
Configured with: ../src/configure -v
--enable-languages=c,c++,java,f77,pascal,objc,ada,treelang --prefix=/usr
--libexecdir=/usr/lib --with-gxx-include-dir=/usr/include/c++/3.4
--enable-shared --with-system-zlib --enable-nls --without-included-gettext
--program-suffix=-3.4 --enable-__cxa_atexit --enable-libstdcxx-allocator=mt
--enable-clocale=gnu --enable-libstdcxx-debug --enable-java-gc=boehm
--enable-java-awt=gtk --disable-werror i486-linux
Thread model: posix
gcc version 3.4.2 (Debian 3.4.2-2)

This is the gcc-3.4 package in debian, FYI, but this bug is not likely to be
debian-specific.

I'm excited to see the -Winit-self warning, but it fails to catch a common
problem, as illustrated by this code fragment (which I will include inline and
also as an attachment for convenience).  This code has two cases of variables
initialized with themselves: one trivial int b = b case, and one more subtle A()
: a(a) case.  g++ warns for one but not the other.  Use this command:

g++-3.4 -c -Winit-self -Wuninitialized -O1 a.cc

on this file:
class A
{
  public:
    A() : a(a)  // <-- should generate a warning
    {
    }
    int getA()
    {
	int b = b;  // <-- generates warning as it should
	return this->a + b;
    }

  private:
    int a;
};

int main()
{
    A a;
    return a.getA();
}


generates this output:

a.cc: In function `int main()':
a.cc:9: warning: 'b' might be used uninitialized in this function

It fails to warn for A::a being initialized with itself in the constructor.

One might also consider it to be a problem that A::getA's being inlined causes
the warning message to mention main() rather than getA() -- this problem is not
present if getA is not inlined.  Anyway, the line number is okay, so this seems
pretty unimportant though still worthy of mention.

It would also be nice if -Winit-self would automatically add -Wuninitialized
instead of just not doing anything unless -Wuninitialized was also specified. 
I'll mention that in a separate bug report.
Comment 1 ejb 2004-10-15 16:46:19 UTC
Created attachment 7358 [details]
source file that reproduces problem

This attachment contains the code that is also inlined in the bug report.
Comment 2 Andrew Pinski 2004-10-15 17:05:57 UTC
-Winit-self has nothing to do with this problem really.

in this case :a(a) is equivalent to this->a = this->a;

We should warn about this case even without -Winit-self or even -Wuninitialize as we can warn without
optimization turned on.

Note I added -Winit-self so I know what it was designed to do.
Comment 3 Giovanni Bajo 2004-10-28 03:35:43 UTC
> -Winit-self has nothing to do with this problem really.
> in this case :a(a) is equivalent to this->a = this->a;

Not really. The member-list syntax is used to *initialize* the members, not to 
assign a value to them after a default initialization.

I think it makes sense to warn only with -Winit-self.
Comment 4 Wolfgang Bangerth 2004-10-28 13:08:42 UTC
That is my view, too. It's an initializer, not an assignment. 
W. 
Comment 5 Geoffrey Irving 2007-04-27 16:45:17 UTC
Is there any chance of activity on this bug?
It would be wonderful to have a warning for this
case, since these bugs can be extremely annoying to find.

If the infrastructure supports it, the ideal way to resolve this might
be to manually mark all fields of this uninitialized on entry to each
constructor.  If that's impossible because the dataflow is run only on
top level variables, just checking for occurences of :a(a) would help
a lot.
Comment 6 Manuel López-Ibáñez 2010-02-21 19:17:00 UTC
(In reply to comment #5)
> Is there any chance of activity on this bug?
> It would be wonderful to have a warning for this
> case, since these bugs can be extremely annoying to find.

-Winit-self is generally broken for C++ (PR 34772). I am not sure what should be solved first.
Comment 7 Jonathan Wakely 2010-12-16 16:05:04 UTC
(In reply to comment #0)
>     A() : a(a)  // <-- should generate a warning

Clang warns about this with -Wuninitialized

My patch for PR 2972 *doesn't* help here, because A::a does have an initializer
Comment 8 Jonathan Wakely 2010-12-21 15:28:18 UTC
With the patch at http://gcc.gnu.org/ml/gcc-patches/2010-12/msg01622.html
the testcase above gives:

$ g++4x a.cc -c -Wuninitialized
a.cc: In constructor ‘A::A()’:
a.cc:4:5: warning: ‘A::a’ is initialized with itself [-Wuninitialized]
a.cc: In member function ‘int A::getA()’:
a.cc:9:10: warning: ‘b’ is used uninitialized in this function [-Wuninitialized]
Comment 9 Jonathan Wakely 2010-12-21 17:19:57 UTC
my patch doesn't help in these cases (which clang does warn about):
A() : a(this->a) { }
A() : a((int)a) { }
A() : a(a+1) { }
For that we need proper tracking of uninitialized variables, which we don't do for member variables.  But my patch catches the simple typo where you accidentally use the wrong variable name in a mem-initializer.
Comment 10 Manuel López-Ibáñez 2010-12-22 09:17:42 UTC
Like others commenting here, I don't understand why a(a) should not warn only with -Winit-self. On the other hand, I always thought that Winit-self is a bad idea. Although the patch does not fixes many cases and thus, we shouldn't close this PR, it is better than nothing. 

About the location, don't we have a better location at that point?

I am thinking that

X() :
j(j), #2
i(i)  #3
{}

should give warnings in #2 and #3.

Please, resubmit and ping. I think this is small enough to go in GCC 4.6.
Comment 11 Jonathan Wakely 2010-12-22 14:40:07 UTC
(In reply to comment #10)
> Like others commenting here, I don't understand why a(a) should not warn only
> with -Winit-self.

I agree with Andrew, the a(a) mistake should always warn, it should be independent of -Winit-self, which exists so that -Wuninitialized doesn't warn about the common (but questionable) practice of self-initializing automatic variables to silence warnings.

As I said in my mail to gcc-patches, if you want to leave a member variable uninitialized, just don't give it a mem-initializer in the constructor.  Giving it a self-initializing one is just perverse.  (The case of automatic variables is different, you can't just not declare it to leave it uninitialized.)

Also, as -Winit-self is broken I didn't want to tie this bug to a broken feature that might be changed to not work for C++.

> On the other hand, I always thought that Winit-self is a bad
> idea. Although the patch does not fixes many cases and thus, we shouldn't close
> this PR, it is better than nothing. 
> 
> About the location, don't we have a better location at that point?
> 
> I am thinking that
> 
> X() :
> j(j), #2
> i(i)  #3
> {}
> 
> should give warnings in #2 and #3.

There are various open bugs about that, e.g. PR 43064, I don't think it's possible at the moment.
Comment 12 Jonathan Wakely 2011-05-23 08:15:24 UTC
Author: redi
Date: Mon May 23 08:15:16 2011
New Revision: 174058

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=174058
Log:
2011-05-23  Jonathan Wakely  <jwakely.gcc@gmail.com>

	PR c++/18016
	* init.c (perform_member_init): Check for self-initialization.

Added:
    trunk/gcc/testsuite/g++.dg/warn/pr18016.C
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/init.c
    trunk/gcc/testsuite/ChangeLog
Comment 13 Jonathan Wakely 2011-05-23 08:19:38 UTC
fixed for 4.7.0
Comment 14 ejb 2011-06-27 18:06:15 UTC
Very nice to see this bug fixed. :-)
Comment 15 Jonathan Wakely 2011-12-13 19:55:11 UTC
*** Bug 51533 has been marked as a duplicate of this bug. ***
Comment 16 Jonathan Wakely 2012-11-14 11:40:50 UTC
*** Bug 55318 has been marked as a duplicate of this bug. ***