Some architectures need "signed char" declarations

Oliver Elphick <olly@lfix.co.uk> writes:

I recently got a Debian bug report about 3 architectures where char is
unsigned by default. There were 2 locations identified in the code
where a char is compared with a negative value, and should therefore be
declared as a "signed char". There may be others in 7.2, but I don't
myself have access to a suitable machine for testing.

The locations I am aware of are:

src/backend/libpq/hba.c GetCharSetByHost(): if (c == EOF)
src/backend/utils/init/miscinit.c SetCharSet(): if (c == EOF)

The architectures are Linux on: arm, powerpc and s390.

Hmmm, according to my knowledge of C, 'c' should be an int here, as
EOF is guaranteed not to collide with any legal char value. With 'c'
an unsigned char, ASCII 255 and EOF would be indistingushable on
twos-complement machines.

-Doug
--
Let us cross over the river, and rest under the shade of the trees.
--T. J. Jackson, 1863

Doug McNaught <doug@wireboard.com> writes:

Hmmm, according to my knowledge of C, 'c' should be an int here, as
EOF is guaranteed not to collide with any legal char value.

I agree with Doug: EOF is not supposed to be equal to any value of
'char', therefore changing the variables to signed char will merely
break something else. Probably the variables should be int; are their
values coming from getc() or some such? Will look at it.

regards, tom lane

Tom Lane wrote:

Doug McNaught <doug@wireboard.com> writes:

Hmmm, according to my knowledge of C, 'c' should be an int here, as
EOF is guaranteed not to collide with any legal char value.

I agree with Doug: EOF is not supposed to be equal to any value of
'char', therefore changing the variables to signed char will merely
break something else. Probably the variables should be int; are their
values coming from getc() or some such? Will look at it.

I deleted the original post, but I think the issue was signed
versus unsigned comparisons. I think he was saying the
variable should be explicitly declared as 'signed int'
(or signed char) and not 'int' (or char) because EOF is (-1).

unsigned int foo;

if (foo == -1) ... causes a warning (or errors)
on many compilers.

And if the default for int or char is unsigned as it can
be on some systems, the code does exactly that.

Perhaps he is just wanted to reduce the build time noise?

Apologies if this was not on point.

Oliver Elphick <olly@lfix.co.uk> writes:

I recently got a Debian bug report about 3 architectures where char is
unsigned by default. There were 2 locations identified in the code
where a char is compared with a negative value, and should therefore be
declared as a "signed char". There may be others in 7.2, but I don't
myself have access to a suitable machine for testing.

The locations I am aware of are:

src/backend/libpq/hba.c GetCharSetByHost(): if (c =3D=3D EOF)
src/backend/utils/init/miscinit.c SetCharSet(): if (c =3D=3D EOF)

Fix committed. I looked at every use of "EOF" in the distribution, and
those two are the only ones I could find that were wrong. I did also
find a place where the result of "getopt" was incorrectly stored in a
"char".

regards, tom lane

Doug Royer <Doug@royer.com> writes:

I deleted the original post, but I think the issue was signed
versus unsigned comparisons. I think he was saying the
variable should be explicitly declared as 'signed int'
(or signed char) and not 'int' (or char) because EOF is (-1).

unsigned int foo;

if (foo == -1) ... causes a warning (or errors)
on many compilers.

And if the default for int or char is unsigned as it can
be on some systems, the code does exactly that.

Perhaps he is just wanted to reduce the build time noise?

Apologies if this was not on point.

The point is that this is potentially buggy code.

-Doug
--
Let us cross over the river, and rest under the shade of the trees.
--T. J. Jackson, 1863

Doug Royer <Doug@royer.com> writes:

And if the default for int or char is unsigned as it can
be on some systems, the code does exactly that.

There are no systems where "int" means "unsigned int". That would break
(to a first approximation) every C program in existence, as well as
violate the ANSI C specification.

The variables in question do need to be "int" not any flavor of "char",
but we don't need to say "signed int".

regards, tom lane