Thread: Some architectures need "signed char" declarations
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. -- Oliver Elphick Oliver.Elphick@lfix.co.uk Isle of Wight http://www.lfix.co.uk/oliver GPG: 1024D/3E1D0C1C: CA12 09E0 E8D5 8870 5839 932A 614D 4C34 3E1D 0C1C "And not only so, but we glory in tribulations also; knowing that tribulation worketh patience; And patience,experience; and experience, hope; And hope maketh not ashamed; because the love of God is shed abroadin our hearts by the Holy Ghost which is given unto us." Romans 5:3-5
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.
Attachment
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