Thread: 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
Attachment
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