Re: [patch] [C/C++] Add a new warning flag -Wself-assign

[Le-Chun Wu <lcwu at google dot com>]

On Wed, Jun 23, 2010 at 3:37 PM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> On Wed, 23 Jun 2010, Le-Chun Wu wrote:
>
>> Hi,
>>
>> Here is the revised patch for the implementation of -Wself-assign
>> warning. Besides minor changes that address the review comments, the
>> major differences of this revised patch from the original one are (1)
>> -Wself-assign is enabled by -Wall, and (2) -Winit-self flag is
>> removed.
>>
>> (For the discussion thread of the original patch, please see
>> http://gcc.gnu.org/ml/gcc-patches/2010-06/msg00974.html)
>
> I see nothing in that thread that justifies breaking backwards
> compatibility by removing an option. ?The usual practice for non-semantic
> options (those not affecting how source code is interpreted or placing
> specific requirements on generated code) would be to keep as a no-op; this
> was done for -Wunreachable-code, for example. ?It might perhaps be better
> to make -Winit-self an alias for the new option, as the nearest current
> equivalent.

Yes, I agree we should not remove -Winit-self and break backwards
compatibility. I will put the option back. However, in this case I
think it's probably better to make it a no-op because -Winit-self used
to only work with -Wuninitialized. (i.e. Without using
-Wuninitialized, -Winit-self is a no-op.) Now that we remove the
functionality of -Winit-self, people who use -Wuninitialized will
always get a warning on the following code with or without
-Winit-self:

void func() {
  int i = i;
}

And also by not making it an alias of -Wself-assign, people who use
-Winit-self won't be surprised by the new warning on self-assignment
statements.

I will send out a revised patch later.

>
>> ? ? ? * gcc/doc/invoke.texi (-Wall): Include -Wself-assign.
>
> Please give the ChangeLog entries separately for each affected ChangeLog
> file. ?This one for example would go in gcc/ChangeLog and the entry would
> start "* doc/invoke.texi". ?The logs in gcc/c-family and gcc/testsuite and
> gcc/cp would also have entries, with the paths being relative to the
> relevant ChangeLog directories.

Yes. I actually have separate ChangeLog files in my svn tree (and Eric
mention that to me as well). I was simply lazy and reused the
ChangeLog entry I sent out in my original patch. :-) I will send out
separate ChangeLog entries in my revised patch.

>
>> +@item -Wself-assign
>> +@opindex Wself-assign
>> +@opindex Wno-self-assign
>> +Warn about self-assignment and self-initialization. This warning is intended
>> +for detecting accidental self-assignment due to typos, and therefore does
>> +not warn on a statement that is semantically a self-assignment after
>> +constant folding. Here is an example of what will trigger a self-assign
>> +warning and what will not:
>> +
>> +@smallexample
>> +@group
>> +void func()
>> +@{
>> + ? int i = 2;
>> + ? int x = x; ? /* warn */
>> + ? float f = 5.0;
>> + ? double a[3];
>> +
>> + ? i = i + 0; ? /* not warn */
>
> Note that an *initialization*, as opposed to an *assignment*, using i + 0
> as the initializer for i (or i + 1, for that matter), is an example of
> erroneous code (using an uninitialized value) people may well want a
> warning for and can get one for right now with -Winit-self. ?Can they
> still get such a warning with the new option?

The new flag doesn't warn on "int i = i + 0", but -Wuninitialized does
(which is a more appropriate option to control the warning on this
case, I think).

$ cat self-init.c
void func()
{
  int a = a + 0;
  int b = b + 1;
  int c;
  c = c + 0;
}

$ gcc -c self-init.c -Wuninitialized
self-init.c: In function ‘func’:
self-init.c:3:7: warning: ‘a’ is used uninitialized in this function
[-Wuninitialized]
self-init.c:4:7: warning: ‘b’ is used uninitialized in this function
[-Wuninitialized]
self-init.c:6:5: warning: ‘c’ is used uninitialized in this function
[-Wuninitialized]

So I think we are covered here.

>
> (The logic above seems fine for which *assignments* get warnings for being
> self-assignments. ?Though if i is uninitialized, an assignment i = i + 0
> should still get a warning for the uninitialized use of i, and I hope it
> does; that warning doesn't depend on -Winit-self at present.)

Yes, it does. See the example above.

Thanks,

Le-chun