Re: coreutils tr cleanup for integer types etc.

[Jim Meyering]

Paul Eggert <address@hidden> wrote:
> I read coreutils CVS tr.c and noticed a lot of minor gotchas
> with integer types, some of which lead to incorrect behavior.
> For example, here's the behavior with x86 coreutils 5.2.1 tr:
> each command behaves incorrectly.
>
>    $ echo a | tr a '[x*][y*2147483646][y*2147483646][y*4]'
>    x
>    $ echo abcd | tr abc '[b*\9]'
>    bbbd
>    $ echo abcd | tr abc '[b*0]'
>    tr: invalid repeat count `0' in [c*n] construct
>    $ echo abcd | tr -c '[a*65536]\n' '[b*]'
>    tr: ../../coreutils-5.2.1/src/tr.c:1942: main: Assertion `get_next (s2, 
> ((void *)0)) == -1 || truncate_set1' failed.
>    Aborted
>
> Here is a patch.
>
> 2004-05-29  Paul Eggert  <address@hidden>
>
>       tr cleanup, mostly having to do with integer type ranges.
>       Remove all casts.

Thanks a lot!  It's certainly humbling to find latent bugs
in code I thought was solid.  I've applied your patch.

I know I'm looking the gift horse in the mouth again,
but if you can find the energy next time, would you please
try to separate the clean-up-style changes from those that
actually fix bugs?

FYI, I've had to apply an additional change to work around
what looks like a bug in HP's /bin/cc compiler.

2004-05-30  Jim Meyering  <address@hidden>

        Work around HPUX /bin/cc compiler bug that is exposed, now that
        sets are arrays of type `bool'.  More details here:
        http://lists.gnu.org/archive/html/bug-gnulib/2004-05/msg00094.html
        FIXME: verify that the above URL points to the right message

        * src/tr.c (card_of_complement): Use cleaner `sizeof in_set'
        rather than `N_CHARS * sizeof(in_set[0])'.  Using HPUX's /bin/cc
        (aC++/ANSI C B3910B A.05.55 [Dec 04 2003]) on an ia64-hp-hpux11.22
        system, those two expressions are not the same (256 vs. 1024).
        The effect of this problem was that `tr -c x y' would fail:
        tr: when not truncating set1, string2 must be non-empty
        (set_initialize): Remove unnecessary initialization of the `in_set'
        buffer; that initialization triggered the same compiler bug as above.

--- tr.c-orig   2004-05-31 20:32:00.222507174 +0200
+++ tr.c        2004-05-31 20:33:07.150705574 +0200
@@ -1200,7 +1200,7 @@ card_of_complement (struct Spec_list *s)
   int cardinality = N_CHARS;
   bool in_set[N_CHARS];
 
-  memset (in_set, 0, N_CHARS * sizeof (in_set[0]));
+  memset (in_set, 0, sizeof in_set);
   s->state = BEGIN_STATE;
   while ((c = get_next (s, NULL)) != -1)
     {
@@ -1664,10 +1664,11 @@ read_and_xlate (char *buf, size_t size)
   return bytes_read;
 }
 
-/* Initialize a boolean membership set IN_SET with the character
+/* Initialize a boolean membership set, IN_SET, with the character
    values obtained by traversing the linked list of constructs S
-   using the function `get_next'.  If COMPLEMENT_THIS_SET is
-   true the resulting set is complemented.  */
+   using the function `get_next'.  IN_SET is expected to have been
+   initialized to all zeros by the caller.  If COMPLEMENT_THIS_SET
+   is true the resulting set is complemented.  */
 
 static void
 set_initialize (struct Spec_list *s, bool complement_this_set, bool *in_set)
@@ -1675,7 +1676,6 @@ set_initialize (struct Spec_list *s, boo
   int c;
   size_t i;
 
-  memset (in_set, 0, N_CHARS * sizeof (in_set[0]));
   s->state = BEGIN_STATE;
   while ((c = get_next (s, NULL)) != -1)
     in_set[c] = true;