Notes on -Wshadow patches

Andrey Smirnov
2011-11-27T12:27:45Z

I finally finished squashing and reordering patches so here are some
notes, before I start sending them.

* Patches format and quantity

- To preserve a backward compatibility in numbering, I decided not to change
anything about previously sent 34 patches.

- Since the number of people expressed concern about the number of
patches they will have to review, I augmented the criteria I used to
split or condense all the other patches. Now all of them must(ideally)
satisfy following criteria:
1. Patch never crosses the boundary of a file
2. Patch is either a series of identical re-namings across
multiple functions or a series of disparate changes within a
boundaries of a single function.
This "algorithm" still yielded 195[1] patches. Because no one provided me
with any specific upper boundary, this unintentionally round number
will have to do for now. Taking into account that all this patch
juggling is quite tedious and time consuming business, I ask anyone,
who wants me to do another round of it, to provide either a new
criterion or an augmentation to the old one from the aforementioned
list, or, if he or she so pleases, they can go grab the sources from
https://github.com/ndreys/gdb/tree/Wshadow-compilation and do it
themselves. These are the only two choices, any other option I will,
most likely, decline.

- To not completely loose my mind while reordering and merging the
patches I allowed myself a luxury of not making an entry in ChangeLog
file itself, but rather in a git commit message(which will still be a
part of an e-mail but not the actual diff). This allowed me to
perform reordering of 300+ patches without it turning into a swearing
fest. Since, in my understanding, due to constant flow of changes to
ChangeLog file, it is impossible to generate correct diff and one will
have to solve merging problems, and put an appropriate date, by hands
anyway, I don't see any problem with my decision. If there's something
I'm missing, and the inclusion of ChangeLog contents in a diff is
unavoidable, let me know, and I will make appropriate changes.

* Some statistics

The contents below is an excerpt from a larger document available at
https://gist.github.com/1397264. It you want all the gory details, go
grab that gist. It is an org-mode file, so I recommend using Emacs to
view it.

Since Mark questioned the usability of -Wshadow option I decided to
look at the full set of data and figure out if it was actually the
case. I decided to investigate -Wshadow generate errors from two
aspects: the cause of the clash and the type of it. So I devised the
following groups of clashes:

Clashes by cause:
- INDX: Local variable shadows `index'
- SGNL: Local variable shadows `signal'
- OIND: Local variables shadow `optind' and `optparse'
- TEEE: Local variable shadows `tee'
- LINK: Local variable shadows `link'
- READ: Local variable shadows `read'
- WRTE: Local variable shadows `write'
- BSNM: Local variable shadows `basename'
- EXPP: Local variable shadows `exp'
- DUPP: Local variable shadows `dup'
- SYSC: Local variable shadows `syscall'
- FSTT: Local variable shadows `fstat'
- STAT: Local variable shadows `stat'
- TTNM: Local variable shadows `ttyname'
- FORK: Local variable shadows `fork'
- WCTP: Local variable shadows `wctype'
- ISTY: Local variable shadows `isatty'
- RWND: Local variable shadows `rewind'
- FPTS: Local variable shadows `fputs'
- SYSN: Local variable shadows `sysinfo'
- ACCS: Local variable shadows `access'
- MISC: All the other name clashes, local to the GDB code.

Clashes by type:
- TYP1: Local variable in a nested scope shadows local variable from outer
scope. Types do not match.
- TYP2: Local variable in a nested scope shadows a function(except
when variable is a pointer to function).
- TYP3: Local variable or function parameter shadows global variable
of different type
- TYP4: Local variable or function parameter shadows type defined with typedef
- ALBW: Local variable in a nested scope shadows local variable from outer
scope. Types match or types are very close(i. e. `int' and `unsigned').
- BWDH: Local variable or function parameter shadows global variable
of the same type

Here's the numbers my investigation produced:

Table 1: Clashes by cause
|-----------+-----+------------|
| Cause | # | % |
|-----------+-----+------------|
| INDX | 28 | 17.07 |
| SGNL | 7 | 4.27 |
| OIND | 7 | 4.27 |
| ERRR | 8 | 4.88 |
| TEEE | 1 | 0.61 |
| LINK | 3 | 1.83 |
| READ | 5 | 3.05 |
| WRTE | 10 | 6.10 |
| BSNM | 3 | 1.83 |
| EXPP | 3 | 1.83 |
| DUPP | 1 | 0.61 |
| SYSC | 2 | 1.22 |
| FSTT | 2 | 1.22 |
| STAT | 1 | 0.61 |
| TTNM | 1 | 0.61 |
| FORK | 1 | 0.61 |
| WCTP | 1 | 0.61 |
| ISTY | 1 | 0.61 |
| RWND | 1 | 0.61 |
| FPTS | 1 | 0.61 |
| SYSN | 2 | 1.22 |
| ACCS | 1 | 0.61 |
|-----------+-----+------------|
| Subtotal: | 90 | 32.93 |
|-----------+-----+------------|
| MISC | 110 | 67.07 |
|-----------+-----+------------|
| Total: | 200 | 100 |
|-----------+-----+------------|

Table 2: Clashes by type
|-----------+-----+------------|
| Cause | # | % |
|-----------+-----+------------|
| TYP1 | 10 | 5.13 |
| TYP2 | 109 | 55.90 |
| TYP3 | 2 | 1.03 |
| TYP4 | 2 | 1.03 |
| ALBW | 59 | 30.26 |
| BWDH | 13 | 6.67 |
|-----------+-----+------------|
| Total: | 195 | 100 |
|-----------+-----+------------|

* My conclusions from the data
- As it can be seen from Table 1, half of the time -Werror reports
errors related to the GDB source code itself, and has nothing to do
with header files provided by the platform.
- 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.

Andrey Smirnov

[1] This makes total of 238 patches: 34 previously sent ones + 195
patches against sources in gdb directory + 8 patches unrelated to
gdb directory + 1 patch to add -Wshadow to default set of flags.