Re: Endian options for "od"

[Eric Blake]

> Hello,
> 
>      It's well-known that "od" displays different output
> depending on whether your CPU is big-endian or little-endian.
> With this simple patch, "od -I" is little-endian on all CPUs,
> and "od -M" is big-endian.  (Think "Intel" and "Motorola").

Thanks for the patch, but it has several problems.  First, I'm
not sure we need short options, or that the documentation
need call out Intel and Motorola (after all, there are other
chip manufacturers as well, Motorola has spun off their
chip division to Freescale, etc.).  Providing just long options
may be an easier task to convince us to add the patch.

> 
>      This patch is very useful for reading files with the
> "wrong" byte order.  "dd conv=swab" only works with shorts,
> not ints, floats, or doubles.
> 
>      "od" with neither option uses CPU byte order (same as
> before), while "od -I -M" uses the opposite of CPU order.

That doesn't quite make sense to have the use of two
contradictory options invoke yet a third behavior.  In my mind,
it would make more sense to have a single long option,
--endian=MODE, where MODE is natural, big, little, or swapped,
rather than having --big-endian --little-endian imply swapped.

Also, a patch like this is incomplete without also touching NEWS
and doc/coreutils.texi.  And it is big enough that you will have
to sign copyright disclaimer before it can be considered for
inclusion.

> @@ -271,7 +274,7 @@
>  #define MAX_FP_TYPE_SIZE sizeof (LONG_DOUBLE)
>  static enum size_spec fp_type_size[MAX_FP_TYPE_SIZE + 1];
>  
> -static char const short_options[] = "A:aBbcDdeFfHhIij:LlN:OoS:st:vw::Xx";
> +static char const short_options[] = "A:aBbcDdeFfHhIij:LlMN:OoS:st:vw::Xx";

Wait a minute - you are proposing adding -I, but -I already exists.

> @@ -1203,7 +1211,14 @@
>           format_address (current_offset, '\0');
>         else
>           printf ("%*s", address_pad_len, "");
> -       (*spec[i].print_function) (n_bytes, curr_block, spec[i].fmt_string);
> +       if (byte_swap && (disp_block = xmalloc (n_bytes))) {

GNU coding standards put the brace on its own line.  Match the
existing coding style.  And mallocing a small block in a for loop
is VERY slow.  It is a small block, so you should be able to allocate
it once on the stack (don't even use alloca; just declare an array
variable that is big enough).

> +         width = width_bytes[spec[i].size];
> +         for (j=0; j < n_bytes; j++)
> +           disp_block[j] = curr_block[j + width-1 - j % width * 2];
> +         (*spec[i].print_function) (n_bytes, disp_block, spec[i].fmt_string);
> +         free (disp_block);
> +       } else
> +         (*spec[i].print_function) (n_bytes, curr_block, spec[i].fmt_string);
>         if (spec[i].hexl_mode_trailer)
>           {
>             /* space-pad out to full line width, then dump the trailer */
> @@ -1682,6 +1697,18 @@
>         flag_dump_strings = true;
>         break;
>  
> +     case 'I':
> +#ifdef WORDS_BIGENDIAN
> +       byte_swap = true;
> +#endif
> +       break;
> +
> +     case 'M':
> +#ifndef WORDS_BIGENDIAN
> +       byte_swap = true;
> +#endif
> +       break;
> +

See my comments above about this being non-intuitive.  I
would expect that if I aliased od to 'od --big-endian' that
I could then do 'od --little-endian' to override my alias, but
as coded, that is not the case.

>       case 't':
>         modern = true;
>         ok &= decode_format_string (optarg);
> @@ -1719,7 +1746,7 @@
>       case 'X': /* obsolescent and undocumented alias */
>         CASE_OLD_ARG ('H', "x4"); /* obsolescent and undocumented */
>         CASE_OLD_ARG ('i', "dI");
> -     case 'I': case 'L': /* obsolescent and undocumented aliases */
> +     case 'L': /* obsolescent and undocumented alias */

The comment is wrong once you delete 'I'.  Hmm, how long
has 'I' been undocumented - is it time to clean up od's options
separately from this patch, and document which options are
retired or withdrawn?

--
Eric Blake