This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Notes on -Wshadow patches


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]