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

[christos%zoulas.com@localhost (Christos Zoulas)]

On Jun 12, 11:20pm, vincent%labri.fr@localhost (Aymeric Vincent) wrote:
-- Subject: Re: CVS commit: src/dist/nvi/common

| > -       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.

It is a bug; a character containing 255 is negative. What do you think this
prints?

#include <stdio.h>

int
main(void) {
        char c = (char)255;
        unsigned int x = c;
        printf("%d\n", x);
        return 0;
}

| 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.

The patch is correct and better since:

        1. It does not produce warnings
        2. It fixes a bug
        3. It generates better code in the non-wide scenario.

I don't really care about size, I care about efficiency and correctness.

| 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.

MAX_FAST_KEY is 255, if it set to more it will break things, since the
value cannot be expressed in character.

christos