Re: Notes on -Wshadow patches

[Andrey Smirnov <andrew dot smirnov at gmail dot com>]

On Sun, Nov 27, 2011 at 9:14 PM, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>> Date: Sun, 27 Nov 2011 08:07:42 -0500
>> From: Eli Zaretskii <eliz@gnu.org>
>
>> > ? - At least 1 in 3 detected conflicts was a conflict of either type ALBW or
>> > ? ? BWHD, both of which, I hope we can all agree, are either
>> > ? ? unintentional mistake or a hack too clever for it's own good, which
>> > ? ? makes the later editing of the code into "Operation" game.
>>
>> I actually think that both ALBW and BWHD are perfectly valid C code,
>> and neither unintentional mistakes nor "too clever hacks". ?The only
>> type of clashes that should be fixed are TYP2.
>

> It's all perfectly valid C code.

I completely agree with you, Mark, but my argument was never about
standard compliance of the code. If -Wshadow was about uncovering the
code that is not valid according to the standard, then both not
enabling and enabling of it by default could have potentially broken
the build.

> That's not the point. The point is that certain types of shadowing
> are indicative of bugs in the code. In case of the ALBW type, the
> potential bug is that the programmer intends to modify the variable
> in the outer scope, which doesn't happen because a variable with the
> same name exists in a nested scope. I can remember a few bugs like
> that in the past decade of GDB development. ?So those are worth
> fixing. ?It's somewhat less likely to be a bug in the BWDH case, but
> I think those should be fixed as well (by renaming the global
> variable, turning it into a static variable, or perhaps by getting
> rid of the global variable completely). ?And fixing the TYP1 and
> TYP3 cases is probably a good idea as well, since it will help to
> make the code more readable. ?A -Wshadow option that only warns
> about these 4 types would actually be useful.

And again I completely agree with you on all of the above
points(except the -Wshadow hating part :), although I would argue that
both are bugs waiting to happen when someone decides to
re-write/augment those pieces of code and remove inner variable,
because in that case compiler won't say anything about any leftover
references to it. And the case of BWDH is worse from that point of
view since with ALBW you will probably be aware of shadowing.

> The TYP2 case will never run the risk of being a bug since the
> distinction between a function invocation and variable usage in the C
> language is unambigious.

The danger I see in TYP2 clashes is not about accidental "invocation"
of variable. Of course it would be reported be compiler even without
-Wshadow. But what about the following example:

#include <stdlib.h>
#include <stdio.h>

void foo(void)
{
  printf ("And now I am a function\n");
}

struct container_element {
  /*
     ...


     Some stuff related to
     inner workings of a container

     ...
  */
  void *data;
};

int main(int argc, char *argv[])
{
  struct container_element elmnt;
  {
    char foo[42];

    /*
      ...

      Lots and lots of code

      ....
    */

    /* Now I am a pointer to a buffer */
    elmnt.data = foo;
  }

  elmnt.data = foo;

  ((void (*) (void))elmnt.data)();

  return 0;
}

How is it not of a TYP2 clash and if it is, is it not as dangerous as BWDH
type?

Andrey Smirnov