INT64_MIN and _MAX

On 21/03/15 23:45, Andrew Gierth wrote:

A couple of places (adt/timestamp.c and pgbench.c) have this:

#ifndef INT64_MAX
#define INT64_MAX INT64CONST(0x7FFFFFFFFFFFFFFF)
#endif

#ifndef INT64_MIN
#define INT64_MIN (-INT64CONST(0x7FFFFFFFFFFFFFFF) - 1)
#endif

On the other hand, int8.c uses the INT64_MIN expression directly inline.

On the third hand, INT64_MIN etc. would typically be defined in stdint.h
if it exists.

So wouldn't it make more sense to move these definitions into c.h and
standardize their usage?

I was thinking the same when I've seen Peter's version of Numeric
abbreviations patch. So +1 for that.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

"Petr" == Petr Jelinek <petr@2ndquadrant.com> writes:

So wouldn't it make more sense to move these definitions into c.h and
standardize their usage?

Petr> I was thinking the same when I've seen Peter's version of Numeric
Petr> abbreviations patch. So +1 for that.

Suggested patch attached.

--
Andrew (irc:RhodiumToad)

"Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:

"Petr" == Petr Jelinek <petr@2ndquadrant.com> writes:

So wouldn't it make more sense to move these definitions into c.h and
standardize their usage?

Petr> I was thinking the same when I've seen Peter's version of Numeric
Petr> abbreviations patch. So +1 for that.

Hm, it looks like the same could be said for INT32_MIN and _MAX; some
places use INT_MIN etc., others say "we shouldn't assume int = int32"
and use (-0x7fffffff - 1) or whatever instead.

--
Andrew (irc:RhodiumToad)

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On March 22, 2015 6:19:52 AM GMT+01:00, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote:

"Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:

"Petr" == Petr Jelinek <petr@2ndquadrant.com> writes:

So wouldn't it make more sense to move these definitions into c.h

and

standardize their usage?

Petr> I was thinking the same when I've seen Peter's version of Numeric
Petr> abbreviations patch. So +1 for that.

Hm, it looks like the same could be said for INT32_MIN and _MAX; some
places use INT_MIN etc., others say "we shouldn't assume int = int32"
and use (-0x7fffffff - 1) or whatever instead.

I have been annoyed by this multiple times. I think we should make sure the C99 defines are there (providing values if they aren't) and always use those. We've used them in parts of the tree long enough that it's unlikely to cause problems. Nothing is helped by using different things in other parts of the tree.

Willing to cook up a patch?

--- 
Please excuse brevity and formatting - I am writing this on my mobile phone.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

"Andres" == Andres Freund <andres@anarazel.de> writes:

Hm, it looks like the same could be said for INT32_MIN and _MAX;
some places use INT_MIN etc., others say "we shouldn't assume int =
int32" and use (-0x7fffffff - 1) or whatever instead.

Andres> I have been annoyed by this multiple times. I think we should
Andres> make sure the C99 defines are there (providing values if they
Andres> aren't) and always use those. We've used them in parts of the
Andres> tree long enough that it's unlikely to cause problems. Nothing
Andres> is helped by using different things in other parts of the tree.

Andres> Willing to cook up a patch?

How's this one?

This replaces the one I posted before; it does both INT64_MIN/MAX and
INT32_MIN/MAX, and also int16/int8/uint*. Uses of 0x7fffffff in code
have been replaced unless there was a reason not to, with either INT_MAX
or INT32_MAX according to the type required.

What I have _not_ done yet is audit uses of INT_MIN/MAX to see which
ones should really be INT32_MIN/MAX.

--
Andrew (irc:RhodiumToad)

On Sun, Mar 22, 2015 at 2:26 AM, Andres Freund <andres@anarazel.de> wrote:

I have been annoyed by this multiple times. I think we should make sure the C99 defines are there (providing values if they aren't) and always use those. We've used them in parts of the tree long enough that it's unlikely to cause problems. Nothing is helped by using different things in other parts of the tree.

+1

--
Peter Geoghegan

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

On 2015-03-22 17:20:22 +0000, Andrew Gierth wrote:

This replaces the one I posted before; it does both INT64_MIN/MAX and
INT32_MIN/MAX, and also int16/int8/uint*. Uses of 0x7fffffff in code
have been replaced unless there was a reason not to, with either INT_MAX
or INT32_MAX according to the type required.

Any reason you did that for most of 0x7FFFFFFF, but not for the
corresponding 0xFFFFFFFF/unsigned case? I'd like to either avoid going
around changing other definitions, or do a somewhat systematic job.

What I have _not_ done yet is audit uses of INT_MIN/MAX to see which
ones should really be INT32_MIN/MAX.

I'm doubtful it's worthwhile to do check that all over the codebase...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

"Andres" == Andres Freund <andres@anarazel.de> writes:

This replaces the one I posted before; it does both INT64_MIN/MAX and
INT32_MIN/MAX, and also int16/int8/uint*. Uses of 0x7fffffff in code
have been replaced unless there was a reason not to, with either INT_MAX
or INT32_MAX according to the type required.

Andres> Any reason you did that for most of 0x7FFFFFFF, but not for the
Andres> corresponding 0xFFFFFFFF/unsigned case? I'd like to either
Andres> avoid going around changing other definitions, or do a somewhat
Andres> systematic job.

I didn't replace the 0xFFFFFFFF ones because most or all of them looked
like basically bit-masking operations rather than actually dealing with
the bounds of an unsigned int or uint32. I was specifically looking for
places where literals were being used to represent maximum or minimum
values.

--
Andrew (irc:RhodiumToad)

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andrew Gierth <andrew@tao11.riddles.org.uk> wrote:

I didn't replace the 0xFFFFFFFF ones because most or all of them looked
like basically bit-masking operations rather than actually dealing with
the bounds of an unsigned int or uint32. I was specifically looking for
places where literals were being used to represent maximum or minimum
values.

Well, InvalidSerCommitSeqNo was initially defined to be UINT64_MAX
-- but some buildfarm members didn't know about that so it was
changed to UINT64CONST(0xFFFFFFFFFFFFFFFF). It is very much about
wanting the maximum value for uint64. As the comment says:

* - InvalidSerCommitSeqNo is used to indicate a transaction that
* hasn't committed yet, so use a number greater than all valid
* ones to make comparison do the expected thing

It does seem odd to only define *some* of these constants.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

"Kevin" == Kevin Grittner <kgrittn@ymail.com> writes:

I didn't replace the 0xFFFFFFFF ones because most or all of them
looked like basically bit-masking operations rather than actually
dealing with the bounds of an unsigned int or uint32. I was
specifically looking for places where literals were being used to
represent maximum or minimum values.

Kevin> Well, InvalidSerCommitSeqNo was initially defined to be
Kevin> UINT64_MAX -- but some buildfarm members didn't know about that
Kevin> so it was changed to UINT64CONST(0xFFFFFFFFFFFFFFFF). It is
Kevin> very much about wanting the maximum value for uint64.

That one _is_ changed to UINT64_MAX in my patch.

--
Andrew (irc:RhodiumToad)

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello,

Grep showed me some unfixed usages of bare constant or INT64CONST
as (u)int64 max/min values.

At Tue, 24 Mar 2015 21:57:42 +0000, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote in <87619q6ouh.fsf@news-spur.riddles.org.uk>

"Kevin" == Kevin Grittner <kgrittn@ymail.com> writes:

Kevin> Well, InvalidSerCommitSeqNo was initially defined to be
Kevin> UINT64_MAX -- but some buildfarm members didn't know about that
Kevin> so it was changed to UINT64CONST(0xFFFFFFFFFFFFFFFF). It is
Kevin> very much about wanting the maximum value for uint64.

That one _is_ changed to UINT64_MAX in my patch.

./src/interfaces/ecpg/pgtypeslib/dt.h:

#define DT_NOBEGIN (-INT64CONST(0x7fffffffffffffff) - 1)
#define DT_NOEND (INT64CONST(0x7fffffffffffffff))

./contrib/pgcrypto/imath.h:

#define MP_WORD_MAX 0xFFFFFFFFFFFFFFFFULL

Likewise, bare (u)int32/16 min/max's are found as following.

./contrib/pgcrypto/imath.h:

#define MP_DIGIT_MAX 0xFFFFFFFFULL

..

#define MP_DIGIT_MAX 0xFFFFUL
#define MP_WORD_MAX 0xFFFFFFFFUL

# MP_DIGIT_MAX was wrong in word length. They are in the half
# length of MP_WORD_MAX.

./src/backend/utils/mb/wchar.c
./src/bin/psql/mbprint.c:

return 0xffffffff;

Additional fixes for the above are in the patch attached.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

"Kyotaro" == Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:

Kyotaro> Hello,
Kyotaro> Grep showed me some unfixed usages of bare constant or
Kyotaro> INT64CONST as (u)int64 max/min values.

Kyotaro> ./src/interfaces/ecpg/pgtypeslib/dt.h:

I didn't touch the ecpg stuff since it wasn't too clear that it was safe
to change, but on second look it is.

Kyotaro> ./contrib/pgcrypto/imath.h:

I didn't touch this since it was obviously a library copied from
somewhere else.

Kyotaro> ./src/backend/utils/mb/wchar.c
Kyotaro> ./src/bin/psql/mbprint.c:

return 0xffffffff;

Here 0xffffffff is not a uint or uint32, but a pg_wchar (which is
unsigned int, not uint32). What's needed there is not UINT_MAX but
rather a PG_WCHAR_INVALID or similar definition in pg_wchar.h.

--
Andrew (irc:RhodiumToad)

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers