Обсуждение: INT64_MIN and _MAX
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? -- Andrew (irc:RhodiumToad)
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
>>>>> "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 NumericPetr> 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)
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 theyaren'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.
>>>>> "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 ifthey 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
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
>>>>> "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*. Usesof 0x7fffffff in code>> have been replaced unless there was a reason not to, with either INT_MAX>> or INT32_MAX accordingto the type required. Andres> Any reason you did that for most of 0x7FFFFFFF, but not for theAndres> corresponding 0xFFFFFFFF/unsigned case? I'dlike to eitherAndres> avoid going around changing other definitions, or do a somewhatAndres> 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)
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
>>>>> "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 ratherthan actually>> dealing with the bounds of an unsigned int or uint32. I was>> specifically looking for places whereliterals were being used to>> represent maximum or minimum values. Kevin> Well, InvalidSerCommitSeqNo was initially defined to beKevin> UINT64_MAX -- but some buildfarm members didn't knowabout thatKevin> so it was changed to UINT64CONST(0xFFFFFFFFFFFFFFFF). It isKevin> very much about wanting the maximumvalue for uint64. That one _is_ changed to UINT64_MAX in my patch. -- Andrew (irc:RhodiumToad)
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
diff --git a/contrib/pgcrypto/imath.h b/contrib/pgcrypto/imath.h
index 0a4f0f7..7c30bd4 100644
--- a/contrib/pgcrypto/imath.h
+++ b/contrib/pgcrypto/imath.h
@@ -44,14 +44,14 @@ typedef int mp_result;typedef uint32 mp_digit;typedef uint64 mp_word;
-#define MP_DIGIT_MAX 0xFFFFFFFFULL
-#define MP_WORD_MAX 0xFFFFFFFFFFFFFFFFULL
+#define MP_DIGIT_MAX UINT32_MAX
+#define MP_WORD_MAX UINT64_MAX#elsetypedef uint16 mp_digit;typedef uint32 mp_word;
-#define MP_DIGIT_MAX 0xFFFFUL
-#define MP_WORD_MAX 0xFFFFFFFFUL
+#define MP_DIGIT_MAX UINT16_MAX
+#define MP_WORD_MAX UINT32_MAX#endiftypedef struct mpz
diff --git a/src/backend/utils/mb/wchar.c b/src/backend/utils/mb/wchar.c
index 0cc753e..6ff99e6 100644
--- a/src/backend/utils/mb/wchar.c
+++ b/src/backend/utils/mb/wchar.c
@@ -729,7 +729,7 @@ utf8_to_unicode(const unsigned char *c) (c[3] & 0x3f)); else /*
thatis an invalid code on purpose */
- return 0xffffffff;
+ return UINT32_MAX;}static int
diff --git a/src/bin/psql/mbprint.c b/src/bin/psql/mbprint.c
index e29c619..6b3f17d 100644
--- a/src/bin/psql/mbprint.c
+++ b/src/bin/psql/mbprint.c
@@ -67,7 +67,7 @@ utf8_to_unicode(const unsigned char *c) (c[3] & 0x3f)); else /*
thatis an invalid code on purpose */
- return 0xffffffff;
+ return UINT32_MAX;}
diff --git a/src/interfaces/ecpg/pgtypeslib/dt.h b/src/interfaces/ecpg/pgtypeslib/dt.h
index 145e2b7..028f0df 100644
--- a/src/interfaces/ecpg/pgtypeslib/dt.h
+++ b/src/interfaces/ecpg/pgtypeslib/dt.h
@@ -320,8 +320,8 @@ do { \#ifdef HAVE_INT64_TIMESTAMP
-#define DT_NOBEGIN (-INT64CONST(0x7fffffffffffffff) - 1)
-#define DT_NOEND (INT64CONST(0x7fffffffffffffff))
+#define DT_NOBEGIN INT64_MIN
+#define DT_NOEND INT64_MAX#else#ifdef HUGE_VAL
>>>>> "Kyotaro" == Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes: Kyotaro> Hello,Kyotaro> Grep showed me some unfixed usages of bare constant orKyotaro> 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.cKyotaro> ./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)