Re: [RFC/PATCH] Clean up unused variables (and prepare for `-Wunused-variable' flag)

[Pedro Alves <palves at redhat dot com>]

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