Centralize use of PG_INTXX_MIN/MAX for integer limits

Michael Paquier <michael@paquier.xyz> writes:

A couple of years ago, 62e2a8dc has introduced in c.h a set of limits
(to fix some portability issues from 83ff1618) to make the code more
system-independent. Those are for example PG_INT32_MIN, etc. The core
code now mixes the internal PG_ limits with the system ones. Would we
want to unify a bit the whole thing and replace all the SHRT_MIN/MAX,
LONG_MIN/MAX and such with the internal limit definitions?

I doubt it's really worth the trouble. I did just make such a change in
commit cbdb8b4c0, but it was mostly (a) so that the different ftoiN/dtoiN
functions would look more alike, and (b) because the relevant variables or
result values were actually declared int16, int32, etc. It would be flat
wrong to replace SHRT_MIN or LONG_MIN in a context where it was used to
check whether a value would fit in a variable declared "short" or "long".

I suspect that the buildfarm does not have any more members where
sizeof(int) is 2.

I doubt PG has ever been able to run on two-byte-int hardware. Certainly
not in the buildfarm era.

I am seeing close to 250 places in the core code,
most of them for INT_MIN and INT_MAX.

You'd really need to look at the associated variables to see whether any
of those would be better off as INT32_MIN/MAX.

regards, tom lane

"Michael" == Michael Paquier <michael@paquier.xyz> writes:

Michael> Hi all,

Michael> A couple of years ago, 62e2a8dc has introduced in c.h a set of
Michael> limits (to fix some portability issues from 83ff1618) to make
Michael> the code more system-independent. Those are for example
Michael> PG_INT32_MIN, etc. The core code now mixes the internal PG_
Michael> limits with the system ones. Would we want to unify a bit the
Michael> whole thing and replace all the SHRT_MIN/MAX, LONG_MIN/MAX and
Michael> such with the internal limit definitions?

Of course not. And LONG_MIN/MAX is the obvious example of why not, since
that one does vary between platforms.

INT_MAX is for the max value of an "int". PG_INT32_MAX is for the max
value of an "int32". PG_INT64_MAX is for the max value of an "int64".
LONG_MAX is for the max value of a "long". Simple.

However, as I said at the time of those patches, I did not at that stage
audit all the uses of INT_MIN/MAX to determine which should really have
been INT32_MIN/MAX. Currently, all sorts of things will likely break if
"int" and "int32" are not exactly the same size, but it might still be a
good idea to fix that at some point. (As a recent example,
contrib/intarray uses "int" almost universally, regardless of the fact
that it's meant to be dealing with SQL "integer" values, which should be
int32.)

--
Andrew (irc:RhodiumToad)

On 24/11/2018 13:15, Michael Paquier wrote:

A couple of years ago, 62e2a8dc has introduced in c.h a set of limits
(to fix some portability issues from 83ff1618) to make the code more
system-independent. Those are for example PG_INT32_MIN, etc. The core
code now mixes the internal PG_ limits with the system ones. Would we
want to unify a bit the whole thing and replace all the SHRT_MIN/MAX,
LONG_MIN/MAX and such with the internal limit definitions?

Since we now require C99, we could also just use the system-provided
definitions in <stdint.h>.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 24/11/2018 13:15, Michael Paquier wrote:

A couple of years ago, 62e2a8dc has introduced in c.h a set of limits
(to fix some portability issues from 83ff1618) to make the code more
system-independent. Those are for example PG_INT32_MIN, etc. The core
code now mixes the internal PG_ limits with the system ones. Would we
want to unify a bit the whole thing and replace all the SHRT_MIN/MAX,
LONG_MIN/MAX and such with the internal limit definitions?

Since we now require C99, we could also just use the system-provided
definitions in <stdint.h>.

We require a C99 *compiler*. That's a different thing from assuming
that the contents of /usr/include are C99-compliant.

Admittedly, the days of user-installed copies of gcc being used with
crufty vendor-supplied system headers might be over for most people.
But my animal gaur still has such a configuration, and it hasn't
got stdint.h at all, never mind any particular contents thereof.

regards, tom lane