Re: -Wformat-signedness

Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> writes:

Hi,

> On 2020-10-29 22:37, Thomas Munro wrote:
>> There're probably mostly harmless, being mostly error and debug
>> messages and the like, and considering that eg OID parsing tolerates
>> negative numbers when reading them back in, but for what it's worth:
>> GCC complains about many %d vs %u type mixups if you build with
>> $SUBJECT.
>
> I had looked into this some time ago. I have dusted off my patch
> again. The attached version fixes all warnings for me.

When Dean pointed me this thread[1], I was thinking we need to add the
"-Wformat-signedness" and fix all the existing warnning. Then after some
research, it is not such easy and seems we need some agreement first if
we want to fix them.

> The following are the main categories of issues:
>
> 1. enums are unsigned by default in gcc, so all those internal error
> messages "unrecognized blah kind: %d" need to be changed to %u.

IIUC, we have agreed that we should cast enum to int and continue to use
"%d". At least Tom suggested this and Thomas agreed this [1] and Peter
didn't raise any opposition.

> 2. Various trickery at the boundary of internal counters that are
> unsigned and external functions or views using signed types. These need
> another look.

I also noticed we lack of UNSIGNED INT32/64 SQL type. Changing the
counter to signed looks not good to me as well. This one looks doesn't
have an agreement yet.

> 3. Various messages print signed values using %x formats, which need to
> be unsigned. These might also need another look.
>
> 4. Issues with constants being signed by default. For example, things
> like elog(ERROR, "foo is %u but should be %u", somevar, 55) warns
> because of the constant. Should be changed to something like 55U for
> symmetry, or change the %u to %d. This also reaches into genbki
> territory with all the OID constants being generated.
>
> 5. Some "surprising" but correct C behavior. For example, unsigned
> short is promoted to int (not unsigned int) in variable arguments, so
> needs a %d format.
>
> 6. Finally, a bunch of uses were just plain wrong and should be corrected.

7. __FILE__ in gcc is 'int', but we elog() it with "%u". Should we
change it to "%d"?

> I haven't found anything that is a really serious bug, but I imagine you
> could run into trouble in various ways when you exceed the INT_MAX
> value. But then again, if you use up INT_MAX WAL timelines, you
> probably have other problems. ;-)

Me too, just that want some clean code:) But FWIW, "-Wformat-signedness"
is not supported by clang so far, so if people is using clang, they
still can't benefit from this changes. My soluation (I use clang
everyday) is adding a "gcc-checker" for my c file, if I make such
mistake, it can remind me directly.

[0] https://www.postgresql.org/message-id/874j4yl4cj.fsf%40163.com
[1]
https://www.postgresql.org/message-id/CA%2BhUKGJNUk434tcsVbs5YUGsujZbveo43QcZeWbv0xPzg9us-A%40mail.gmail.com

--
Best Regards
Andy Fan