From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17888 invoked by alias); 24 Apr 2012 11:15:43 -0000 Received: (qmail 17879 invoked by uid 22791); 24 Apr 2012 11:15:42 -0000 X-SWARE-Spam-Status: No, hits=-7.3 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 24 Apr 2012 11:15:27 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q3OBFQL2006231 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 24 Apr 2012 07:15:27 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q3OBFPtm015601; Tue, 24 Apr 2012 07:15:26 -0400 Message-ID: <4F968B4D.3050209@redhat.com> Date: Tue, 24 Apr 2012 12:16:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20120329 Thunderbird/11.0.1 MIME-Version: 1.0 To: Sergio Durigan Junior CC: gdb-patches@sourceware.org Subject: Re: [RFC/PATCH] Clean up unused variables (and prepare for `-Wunused-variable' flag) References: In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8bit 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: 2012-04/txt/msg00769.txt.bz2 Hi Sérgio, First off, thanks for doing this. On 04/23/2012 11:51 PM, Sergio Durigan Junior wrote: > Hi, > > This patch is a followup of the discussion in: > > http://sourceware.org/ml/gdb/2012-04/msg00171.html > > First of all, I am sorry for the size of this patch, but I couldn't > think of a good way of splitting it, and also I thought it would be > useless since these changes are all logically related. It's not useless at all. This warning points at two classes of problems: - variables that are no longer necessary, and can be garbage collected. - variables that actually should be being used, but they're not due to some latent bug. I skimmed the patch, and noted several places, mostly in tdep code, where you end up removing more than the unsuspecting auxiliary and obviously-left- -behind-by-accident variable. Some of those removed bits could well be latent bugs. Some hunks seem to remove used variables and expand what they were initialized from at the used sites. What's up with that? Please give rationale for any change that requires more than idle brain power to understand. :-) Splitting the patch e.g., by architecture (one for arm, one for x86, etc.) should make it easier for the people in charge of those bits to take a proper look. There's no need to commit all this in one go. Once you split, some of the resulting patches will be dead obvious, and will end up reviewed (if even necessary) and checked in quickly, and thus you end up making progress faster that way. > I regtested the patch on a Fedora 16 x86{,_64}, without regressions. I > also built the patch using `--enable-targets=all --enable-plugins > --enable-gold', and everything succeeded. "--enable-plugins --enable-gold" don't mean anything for GDB, AFAIK. > > I'd like to apply it, but I have a couple of questions before: > > a) How's the ChangeLog for this patch supposed to be? Can I make a > "generic" ChangeLog, saying something like `Remove unused variables from > files'? > > b) I'd like someone to take a look at the `observer.sh' change, please. Please sent it as a separate patch, along with a rationale. Also, the .c files under features/ are generated files. We'll need to fix the generator instead, again, best done as a separate patch. -- Pedro Alves