Re: [OT] Is od broken?

[Paul Eggert]

Eric Blake <address@hidden> writes:

> -    printf (fmt_string, *p++);
> +    printf (fmt_string, pad, "", *p++);

How about doing padding this way instead?

        printf (fmt_string, width, *p++);;

I think this would be clearer all-around.  Here is a revised patch
to od.c to do it that way.  The rest of your patch would remain unchanged.

* src/od.c (struct tspec): Add pad_width field, and adjust
print_function prototype.
(decode_one_format): Default to pad width of 0, and rewrite all
fmt_string values to account for pad width.
(FMT_BYTES_ALLOCATED): Adjust to new format style.
(main): Compute pad width per spec.
(write_block): Account for pad width.
(print_s_char, print_char, print_s_short, print_short, print_int)
(print_long, print_long_long, print_float, print_double)
(print_long_double, print_named_ascii, print_ascii): All print
functions adjusted to use pad width.
(charname): Pad values to right with spaces, to simplify pad calculations.
Turn it into a 2D array, since there's no need for pointers now.

diff --git a/src/od.c b/src/od.c
index 4df8e7d..7fb6243 100644
--- a/src/od.c
+++ b/src/od.c
@@ -92,13 +92,13 @@ enum output_format
 enum
   {
     FMT_BYTES_ALLOCATED =
-      MAX ((sizeof " %0" - 1 + INT_STRLEN_BOUND (int)
+      MAX ((sizeof " %0*" - 1
            + MAX (sizeof "ld",
                   MAX (sizeof PRIdMAX,
                        MAX (sizeof PRIoMAX,
                             MAX (sizeof PRIuMAX,
                                  sizeof PRIxMAX))))),
-          sizeof " %.Le" + 2 * INT_STRLEN_BOUND (int))
+          sizeof " %*.Le" + INT_STRLEN_BOUND (int))
   };
 
 /* Each output format specification (from `-t spec' or from
@@ -107,10 +107,11 @@ struct tspec
   {
     enum output_format fmt;
     enum size_spec size;
-    void (*print_function) (size_t, void const *, char const *);
+    void (*print_function) (size_t, void const *, char const *, int);
     char fmt_string[FMT_BYTES_ALLOCATED];
     bool hexl_mode_trailer;
     int field_width;
+    int pad_width; /* Leading spaces; does not include leftmost space.  */
   };
 
 /* Convert the number of 8-bit bytes of a binary representation to
@@ -167,13 +168,13 @@ static const int width_bytes[] =
 verify (sizeof width_bytes / sizeof width_bytes[0] == N_SIZE_SPECS);
 
 /* Names for some non-printing characters.  */
-static const char *const charname[33] =
+static char const charname[33][4] =
 {
   "nul", "soh", "stx", "etx", "eot", "enq", "ack", "bel",
-  "bs", "ht", "nl", "vt", "ff", "cr", "so", "si",
+  "bs ", "ht ", "nl ", "vt ", "ff ", "cr ", "so ", "si ",
   "dle", "dc1", "dc2", "dc3", "dc4", "nak", "syn", "etb",
-  "can", "em", "sub", "esc", "fs", "gs", "rs", "us",
-  "sp"
+  "can", "em ", "sub", "esc", "fs ", "gs ", "rs ", "us ",
+  "sp "
 };
 
 /* Address base (8, 10 or 16).  */
@@ -392,94 +393,102 @@ implies 32.  By default, od uses -A o -t d2 -w16.\n\
 /* Define the print functions.  */
 
 static void
-print_s_char (size_t n_bytes, void const *block, char const *fmt_string)
+print_s_char (size_t n_bytes, void const *block, char const *fmt_string,
+             int width)
 {
   signed char const *p = block;
   size_t i;
   for (i = n_bytes / sizeof *p; i != 0; i--)
-    printf (fmt_string, *p++);
+    printf (fmt_string, width, *p++);
 }
 
 static void
-print_char (size_t n_bytes, void const *block, char const *fmt_string)
+print_char (size_t n_bytes, void const *block, char const *fmt_string,
+            int width)
 {
   unsigned char const *p = block;
   size_t i;
   for (i = n_bytes / sizeof *p; i != 0; i--)
-    printf (fmt_string, *p++);
+    printf (fmt_string, width, *p++);
 }
 
 static void
-print_s_short (size_t n_bytes, void const *block, char const *fmt_string)
+print_s_short (size_t n_bytes, void const *block, char const *fmt_string,
+               int width)
 {
   short int const *p = block;
   size_t i;
   for (i = n_bytes / sizeof *p; i != 0; i--)
-    printf (fmt_string, *p++);
+    printf (fmt_string, width, *p++);
 }
 
 static void
-print_short (size_t n_bytes, void const *block, char const *fmt_string)
+print_short (size_t n_bytes, void const *block, char const *fmt_string,
+            int width)
 {
   unsigned short int const *p = block;
   size_t i;
   for (i = n_bytes / sizeof *p; i != 0; i--)
-    printf (fmt_string, *p++);
+    printf (fmt_string, width, *p++);
 }
 
 static void
-print_int (size_t n_bytes, void const *block, char const *fmt_string)
+print_int (size_t n_bytes, void const *block, char const *fmt_string, int 
width)
 {
   unsigned int const *p = block;
   size_t i;
   for (i = n_bytes / sizeof *p; i != 0; i--)
-    printf (fmt_string, *p++);
+    printf (fmt_string, width, *p++);
 }
 
 static void
-print_long (size_t n_bytes, void const *block, char const *fmt_string)
+print_long (size_t n_bytes, void const *block, char const *fmt_string, int 
width)
 {
   unsigned long int const *p = block;
   size_t i;
   for (i = n_bytes / sizeof *p; i != 0; i--)
-    printf (fmt_string, *p++);
+    printf (fmt_string, width, *p++);
 }
 
 static void
-print_long_long (size_t n_bytes, void const *block, char const *fmt_string)
+print_long_long (size_t n_bytes, void const *block, char const *fmt_string,
+                 int width)
 {
   unsigned_long_long_int const *p = block;
   size_t i;
   for (i = n_bytes / sizeof *p; i != 0; i--)
-    printf (fmt_string, *p++);
+    printf (fmt_string, width, *p++);
 }
 
 static void
-print_float (size_t n_bytes, void const *block, char const *fmt_string)
+print_float (size_t n_bytes, void const *block, char const *fmt_string,
+             int width)
 {
   float const *p = block;
   size_t i;
   for (i = n_bytes / sizeof *p; i != 0; i--)
-    printf (fmt_string, *p++);
+    printf (fmt_string, width, *p++);
 }
 
 static void
-print_double (size_t n_bytes, void const *block, char const *fmt_string)
+print_double (size_t n_bytes, void const *block, char const *fmt_string,
+              int width)
 {
   double const *p = block;
   size_t i;
   for (i = n_bytes / sizeof *p; i != 0; i--)
-    printf (fmt_string, *p++);
+    printf (fmt_string, width, *p++);
 }
 
 #ifdef HAVE_LONG_DOUBLE
 static void
-print_long_double (size_t n_bytes, void const *block, char const *fmt_string)
+print_long_double (size_t n_bytes, void const *block, char const *fmt_string,
+                   int width)
 {
   long double const *p = block;
   size_t i;
   for (i = n_bytes / sizeof *p; i != 0; i--)
-    printf (fmt_string, *p++);
+    printf (fmt_string, width, *p++);
 }
 #endif
 
@@ -499,7 +508,7 @@ dump_hexl_mode_trailer (size_t n_bytes, const char *block)
 
 static void
 print_named_ascii (size_t n_bytes, void const *block,
-                  const char *unused_fmt_string ATTRIBUTE_UNUSED)
+                  const char *unused_fmt_string ATTRIBUTE_UNUSED, int width)
 {
   unsigned char const *p = block;
   size_t i;
@@ -519,13 +528,13 @@ print_named_ascii (size_t n_bytes, void const *block,
          s = buf;
        }
 
-      printf (" %3s", s);
+      printf (" %*s", width, s);
     }
 }
 
 static void
 print_ascii (size_t n_bytes, void const *block,
-            const char *unused_fmt_string ATTRIBUTE_UNUSED)
+            const char *unused_fmt_string ATTRIBUTE_UNUSED, int width)
 {
   unsigned char const *p = block;
   size_t i;
@@ -574,7 +583,7 @@ print_ascii (size_t n_bytes, void const *block,
          s = buf;
        }
 
-      printf (" %3s", s);
+      printf (" %*s", width, s);
     }
 }
 
@@ -614,7 +623,8 @@ simple_strtoul (const char *s, const char **p, unsigned 
long int *val)
        fmt = SIGNED_DECIMAL;
        size = INT or LONG; (whichever integral_type_size[4] resolves to)
        print_function = print_int; (assuming size == INT)
-       fmt_string = "%011d%c";
+       fmt_string = " %0*d";
+       pad_width = 0; (but may be increased later)
       }
    S_ORIG is solely for reporting errors.  It should be the full format
    string argument.
@@ -628,7 +638,7 @@ decode_one_format (const char *s_orig, const char *s, const 
char **next,
   unsigned long int size;
   enum output_format fmt;
   const char *pre_fmt_string;
-  void (*print_function) (size_t, void const *, char const *);
+  void (*print_function) (size_t, void const *, char const *, int);
   const char *p;
   char c;
   int field_width;
@@ -702,29 +712,29 @@ this system doesn't provide a %lu-byte integral type"), 
quote (s_orig), size);
        {
        case 'd':
          fmt = SIGNED_DECIMAL;
-         sprintf (tspec->fmt_string, " %%%d%s",
-                  (field_width = bytes_to_signed_dec_digits[size]),
+         field_width = bytes_to_signed_dec_digits[size];
+         sprintf (tspec->fmt_string, " %%*%s",
                   ISPEC_TO_FORMAT (size_spec, "d", "ld", PRIdMAX));
          break;
 
        case 'o':
          fmt = OCTAL;
-         sprintf (tspec->fmt_string, " %%0%d%s",
-                  (field_width = bytes_to_oct_digits[size]),
+         field_width = bytes_to_oct_digits[size];
+         sprintf (tspec->fmt_string, " %%0*%s",
                   ISPEC_TO_FORMAT (size_spec, "o", "lo", PRIoMAX));
          break;
 
        case 'u':
          fmt = UNSIGNED_DECIMAL;
-         sprintf (tspec->fmt_string, " %%%d%s",
-                  (field_width = bytes_to_unsigned_dec_digits[size]),
+         field_width = bytes_to_unsigned_dec_digits[size];
+         sprintf (tspec->fmt_string, " %%*%s",
                   ISPEC_TO_FORMAT (size_spec, "u", "lu", PRIuMAX));
          break;
 
        case 'x':
          fmt = HEXADECIMAL;
-         sprintf (tspec->fmt_string, " %%0%d%s",
-                  (field_width = bytes_to_hex_digits[size]),
+         field_width = bytes_to_hex_digits[size];
+         sprintf (tspec->fmt_string, " %%0*%s",
                   ISPEC_TO_FORMAT (size_spec, "x", "lx", PRIxMAX));
          break;
 
@@ -817,20 +827,20 @@ this system doesn't provide a %lu-byte floating point 
type"),
        case FLOAT_SINGLE:
          print_function = print_float;
          /* Don't use %#e; not all systems support it.  */
-         pre_fmt_string = " %%%d.%de";
+         pre_fmt_string = "";
          precision = FLT_DIG;
          break;
 
        case FLOAT_DOUBLE:
          print_function = print_double;
-         pre_fmt_string = " %%%d.%de";
+         pre_fmt_string = "";
          precision = DBL_DIG;
          break;
 
 #ifdef HAVE_LONG_DOUBLE
        case FLOAT_LONG_DOUBLE:
          print_function = print_long_double;
-         pre_fmt_string = " %%%d.%dLe";
+         pre_fmt_string = "L";
          precision = LDBL_DIG;
          break;
 #endif
@@ -840,7 +850,7 @@ this system doesn't provide a %lu-byte floating point 
type"),
        }
 
       field_width = precision + 8;
-      sprintf (tspec->fmt_string, pre_fmt_string, field_width, precision);
+      sprintf (tspec->fmt_string, " %%*.%d%se", precision, pre_fmt_string);
       break;
 
     case 'a':
@@ -870,6 +880,7 @@ this system doesn't provide a %lu-byte floating point 
type"),
   tspec->print_function = print_function;
 
   tspec->field_width = field_width;
+  tspec->pad_width = 0;
   tspec->hexl_mode_trailer = (*s == 'z');
   if (tspec->hexl_mode_trailer)
     s++;
@@ -1198,13 +1209,14 @@ write_block (uintmax_t current_offset, size_t n_bytes,
            format_address (current_offset, '\0');
          else
            printf ("%*s", address_pad_len, "");
-         (*spec[i].print_function) (n_bytes, curr_block, spec[i].fmt_string);
+         (*spec[i].print_function) (n_bytes, curr_block, spec[i].fmt_string,
+                                     spec[i].field_width + spec[i].pad_width);
          if (spec[i].hexl_mode_trailer)
            {
              /* space-pad out to full line width, then dump the trailer */
              int datum_width = width_bytes[spec[i].size];
              int blank_fields = (bytes_per_block - n_bytes) / datum_width;
-             int field_width = spec[i].field_width + 1;
+             int field_width = spec[i].field_width + spec[i].pad_width + 1;
              printf ("%*s", blank_fields * field_width, "");
              dump_hexl_mode_trailer (n_bytes, curr_block);
            }
@@ -1555,6 +1567,7 @@ main (int argc, char **argv)
   bool modern = false;
   bool width_specified = false;
   bool ok = true;
+  size_t width_per_lcm = 0;
   static char const multipliers[] = "bEGKkMmPTYZ0";
 
   /* The old-style `pseudo starting address' to be printed in parentheses
@@ -1915,11 +1928,45 @@ it must be one character from [doxn]"),
        bytes_per_block = l_c_m;
     }
 
+  /* Compute padding necessary to align output block.  */
+  if (1 < n_specs)
+    {
+      for (i = 0; i < n_specs; i++)
+        {
+          int fields_per_lcm = l_c_m / width_bytes[spec[i].size];
+          int lcm_width = (spec[i].field_width + 1) * fields_per_lcm;
+          if (width_per_lcm < lcm_width)
+            {
+              width_per_lcm = lcm_width;
+              if (width_per_lcm % l_c_m)
+                width_per_lcm = ((width_per_lcm / l_c_m) + 1) * l_c_m;
+            }
+        }
+      for (i = 0; i < n_specs; i++)
+        {
+          int fields_per_block = bytes_per_block / width_bytes[spec[i].size];
+          int block_width = spec[i].field_width * fields_per_block;
+          spec[i].pad_width = (((width_per_lcm * (bytes_per_block / l_c_m)
+                                - block_width)
+                               / fields_per_block)
+                              - 1);
+        }
+    }
+
 #ifdef DEBUG
+  else
+    width_per_lcm = spec[0].field_width + 1;
+  printf (_("lcm=%d, width_per_lcm=%zu\n"), l_c_m, width_per_lcm);
   for (i = 0; i < n_specs; i++)
     {
-      printf (_("%d: fmt=\"%s\" width=%d\n"),
-             i, spec[i].fmt_string, width_bytes[spec[i].size]);
+      int fields_per_lcm = l_c_m / width_bytes[spec[i].size];
+      assert (bytes_per_block % width_bytes[spec[i].size] == 0);
+      assert (width_per_lcm == ((spec[i].field_width + spec[i].pad_width + 1)
+                                * fields_per_lcm));
+      assert (0 <= spec[i].pad_width && spec[i].pad_width + 1 < width_per_lcm);
+      printf (_("%d: fmt=\"%s\" in_width=%d out_width=%d pad=%d\n"),
+             i, spec[i].fmt_string, width_bytes[spec[i].size],
+              spec[i].field_width, spec[i].pad_width);
     }
 #endif