From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9975 invoked by alias); 27 Nov 2011 12:27:45 -0000 Received: (qmail 9966 invoked by uid 22791); 27 Nov 2011 12:27:43 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from mail-bw0-f41.google.com (HELO mail-bw0-f41.google.com) (209.85.214.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 27 Nov 2011 12:27:29 +0000 Received: by bke17 with SMTP id 17so7718679bke.0 for ; Sun, 27 Nov 2011 04:27:28 -0800 (PST) Received: by 10.204.128.199 with SMTP id l7mr30641255bks.27.1322396847972; Sun, 27 Nov 2011 04:27:27 -0800 (PST) Received: from bulbasaur (l49-9-169.cn.ru. [178.49.9.169]) by mx.google.com with ESMTPS id jf4sm18584569bkc.5.2011.11.27.04.27.25 (version=TLSv1/SSLv3 cipher=OTHER); Sun, 27 Nov 2011 04:27:27 -0800 (PST) From: Andrey Smirnov To: gdb-patches@sourceware.org Cc: eliz@gnu.org, brobecker@adacore.com, mark.kettenis@xs4all.nl, tromey@redhat.com Subject: Notes on -Wshadow patches Date: Sun, 27 Nov 2011 12:27:00 -0000 Message-ID: <877h2l7nth.fsf@gmail.com> MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-11/txt/msg00719.txt.bz2 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.