Обсуждение: Centralize use of PG_INTXX_MIN/MAX for integer limits

Hi all,

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 suspect that the buildfarm does not have any more members where
sizeof(int) is 2.  I am seeing close to 250 places in the core code,
most of them for INT_MIN and INT_MAX.

Thoughts?
--
Michael

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