Re: Misc cleanups

[Ian Lance Taylor <ian at wasabisystems dot com>]

Nathan Sidwell <nathan@codesourcery.com> writes:

> > Also, now that I look more closely at your SKIP_ZEROES change, I don't
> > like it.  SKIP_ZEROES has a value which you should use.  You shouldn't
> > test it with #ifdef.
> it doesn't, it used #if :)
> 
> > It seems to me that you are adding a special case for setting
> > SKIP_ZEROES to 0 to disable it, which I don't understand.  I don't
> Currently setting SKIP_ZEROES to zero would result on the disassembler
> always skipping no zeros! (And consequently, never advancing.)
> Having 0 mean 'don't skip' seemed an obvious meaning to me.
> 
> You're right that one could use -z, and a long sequence of zero
> bytes is nearly never a valid program, but those don't persuade me
> as arguments to prevent a port from doing what I did.

I don't find that wholly confusing, but I appreciate that there may be
some aspects of this port which you aren't able to share with us.

In any case, the whole thing is making me itch, because the current
mechanism for ports to set SKIP_ZEROES is broken.

Nick, you suggested it here:
    http://sources.redhat.com/ml/binutils/2002-10/msg00554.html
but it doesn't make any sense to me.  It's a host/target confusion.

If you do this:
    ../src/configure --enable-targets=c4x-coff,ia64-elf
then when you get around to building objdump.o, you get this:

gcc -DHAVE_CONFIG_H -I. -I../../src/binutils -I. -D_GNU_SOURCE -I. -I../../src/binutils -I../bfd -I../../src/binutils/../bfd -I../../src/binutils/../include -I../../src/binutils/../intl -I../intl -DLOCALEDIR="\"/usr/local/share/locale\"" -Dbin_dummy_emulation=bin_vanilla_emulation   -W -Wall -Wstrict-prototypes -Wmissing-prototypes -g -O2 -c -DSKIP_ZEROES=32 -DSKIP_ZEROES=16 ../../src/binutils/objdump.c
<command line>: warning: "SKIP_ZEROES" redefined
<command line>: warning: this is the location of the previous definition

There are two definitions for SKIP_ZEROES.  Neither applies to the
default target, they merely apply to targets which happen to be
enabled.  This confusion is hidden because people have failed to
notice the --enable-targets=all case.  In this example, what should we
use for SKIP_ZEROES?

The right way to handle this is to add a new field to struct
disassemble_info, similar to the bytes_per_chunk field.  objdump would
set the field to a reasonable default, such as the current default of
8.  The relevant disassembler would then set the field as desired.

I only made SKIP_ZEROES a macro in the first place on the general
principle of ``avoid magic numbers in code.''  It could just as well
have been a const int.  I don't want the fact that I wrote SKIP_ZEROES
as a macro to be used as a reason to test it with #if.

Ian