Re: CVS commit: src/dist/nvi/common

[Aymeric Vincent <vincent%labri.fr@localhost>]

christos%astron.com@localhost (Christos Zoulas) writes:

>>XXX may be not the right thing to do, but very little intrusive
>
> Yes, it is not the right thing to do :-)

Agreed. :-)

> The following patch is probably
> what the author intended. It simplifies the code in the non wide case, and
> fixes a bug in the wide case too:
>
> -         sp->gp->special_key[(UCHAR_T)ch] :                          \
> +         sp->gp->special_key[(unsigned char)ch] :                    \

I don't see how it fixes a bug: this cast is done only if (ch <= 255),
so whatever the signedness of ch, and whatever the actual size of
UCHAR_T, we will get a value comprised between 0 and 255, and of an
unsigned integer type.

IMHO, gcc is being picky in this case, the fact that it can optimize
the test out is a good thing, but it can also do it when ch is a
signed char. In one case it complains, in the other case it doesn't.

Maybe I'm missing something, but I would really like to keep changes
small, that's why I'm arguing. There is no doubt your patch is better
than what I committed because it doesn't "cripple" the array by one
element.

Regards,
 Aymeric

PS: I even think that in the hypothesis where MAX_FAST_KEY would be
    set beyond 255, the (unsigned char) cast would do the wrong
    thing.