Missing PG_INT32_MIN in numutils.c

On Wed, Apr 13, 2016 at 3:49 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

Hi all,

While going through numutils.c I found the following thing:
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -136,7 +136,7 @@ pg_ltoa(int32 value, char *a)
* Avoid problems with the most negative integer not being representable
* as a positive integer.
*/
-   if (value == (-2147483647 - 1))
+   if (value == PG_INT32_MIN)
{
memcpy(a, "-2147483648", 12);
return;
Attached is a patch. The interesting part is that pg_lltoa is not
missing the check on PG_INT64_MIN.

Committed.

--
Robert Haas
EnterpriseDB: 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

On Wed, Apr 13, 2016 at 8:57 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Apr 13, 2016 at 3:49 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

Hi all,

While going through numutils.c I found the following thing:
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -136,7 +136,7 @@ pg_ltoa(int32 value, char *a)
* Avoid problems with the most negative integer not being representable
* as a positive integer.
*/
-   if (value == (-2147483647 - 1))
+   if (value == PG_INT32_MIN)
{
memcpy(a, "-2147483648", 12);
return;
Attached is a patch. The interesting part is that pg_lltoa is not
missing the check on PG_INT64_MIN.

Committed.

Thanks.
--
Michael

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

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Apr 13, 2016 at 3:49 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

While going through numutils.c I found the following thing:
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -136,7 +136,7 @@ pg_ltoa(int32 value, char *a)
* Avoid problems with the most negative integer not being representable
* as a positive integer.
*/
-   if (value == (-2147483647 - 1))
+   if (value == PG_INT32_MIN)
{
memcpy(a, "-2147483648", 12);
return;
Attached is a patch. The interesting part is that pg_lltoa is not
missing the check on PG_INT64_MIN.

Committed.

I am not very convinced that this is an improvement, because you took
what had been two hard-wired constants and replaced them with a symbol
and a hard-wired constant. This is more prone to break, not less so.
If there were a way to stringify PG_INT32_MIN's value for use in the
memcpy (which would then better be strcpy), then converting *both*
constants would be an improvement --- but otherwise I think this was
best left alone.

regards, tom lane

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

On Wed, Apr 13, 2016 at 9:38 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Apr 13, 2016 at 3:49 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

While going through numutils.c I found the following thing:
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -136,7 +136,7 @@ pg_ltoa(int32 value, char *a)
* Avoid problems with the most negative integer not being representable
* as a positive integer.
*/
-   if (value == (-2147483647 - 1))
+   if (value == PG_INT32_MIN)
{
memcpy(a, "-2147483648", 12);
return;
Attached is a patch. The interesting part is that pg_lltoa is not
missing the check on PG_INT64_MIN.

Committed.

I am not very convinced that this is an improvement, because you took
what had been two hard-wired constants and replaced them with a symbol
and a hard-wired constant.This is more prone to break, not less so.
If there were a way to stringify PG_INT32_MIN's value for use in the
memcpy (which would then better be strcpy), then converting *both*
constants would be an improvement --- but otherwise I think this was
best left alone.

*shrug*

I think it's kind of six of one, half a dozen of the other, but if you
feel strongly about it, revert the patch.

--
Robert Haas
EnterpriseDB: 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

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Apr 13, 2016 at 9:38 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I am not very convinced that this is an improvement, because you took
what had been two hard-wired constants and replaced them with a symbol
and a hard-wired constant.This is more prone to break, not less so.

I think it's kind of six of one, half a dozen of the other, but if you
feel strongly about it, revert the patch.

I don't care enough to do that either, but I wanted to point out that
it's pretty questionable whether this is a stylistic improvement.

regards, tom lane

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

On Wed, Apr 13, 2016 at 10:11 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Apr 13, 2016 at 9:38 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I am not very convinced that this is an improvement, because you took
what had been two hard-wired constants and replaced them with a symbol
and a hard-wired constant.This is more prone to break, not less so.

I think it's kind of six of one, half a dozen of the other, but if you
feel strongly about it, revert the patch.

I don't care enough to do that either, but I wanted to point out that
it's pretty questionable whether this is a stylistic improvement.

Yeah, fair. I think it depends on whether you think it is more likely
that people will (a) grep for PG_INT_MIN32 to find places where we do
overflow handling or (b) observe the close relationship between the
two constants on adjacent lines. Probably I should have waited for
comments before committing, but I figured we wanted to avoid hardcoded
constants and didn't think much further.

--
Robert Haas
EnterpriseDB: 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

On Wed, Apr 13, 2016 at 11:13 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Apr 13, 2016 at 10:11 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Apr 13, 2016 at 9:38 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I am not very convinced that this is an improvement, because you took
what had been two hard-wired constants and replaced them with a symbol
and a hard-wired constant.This is more prone to break, not less so.

I think it's kind of six of one, half a dozen of the other, but if you
feel strongly about it, revert the patch.

I don't care enough to do that either, but I wanted to point out that
it's pretty questionable whether this is a stylistic improvement.

Yeah, fair. I think it depends on whether you think it is more likely
that people will (a) grep for PG_INT_MIN32 to find places where we do
overflow handling or (b) observe the close relationship between the
two constants on adjacent lines. Probably I should have waited for
comments before committing, but I figured we wanted to avoid hardcoded
constants and didn't think much further.

Well, I think that's an improvement as well when looking for places
checking for overflows. And if you revert the patch, you may want to
look as well at pg_lltoa that does the same business with PG_INT64_MIN
and not a hardcoded constant.
--
Michael

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