Thread: Missing PG_INT32_MIN in numutils.c
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.
Regards,
--
Michael
Attachment
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
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
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
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
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
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
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