Re: refactoring - share str2*int64 functions

From: Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Andres Freund <andres(at)anarazel(dot)de>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: refactoring - share str2*int64 functions
Date: 2019-08-30 08:06:11
Message-ID: alpine.DEB.2.21.1908300943110.28828@lancre
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers


Michaël,

> attached is a first cut that I would like to commit separately which
> adds all the compatibility overflow routines to int.h for uint16, uint32
> and uint64 with all the fallback implementations (int128-based method
> added as well if available). I have also grouped at the top of the file
> the comments about each routine's return policy to avoid duplication.
> For the fallback implementations of uint64 using int128, I think that it
> is cleaner to use uint128 so as there is no need to check after negative
> results for multiplications, and I have made the various expressions
> consistent for each size.

Patch applies cleanly, compiles, "make check" ok, but the added functions
are not used (yet).

I think that factoring out comments is a good move.

For symmetry and efficiency, ISTM that uint16 mul overflow could use
uint32 and uint32 could use uint64, and the division-based method be
dropped in these cases.

Maybe I would add a comment before each new section telling about the
type, eg:

/*
* UINT16
*/
add/sub/mul uint16 functions…

I would tend to commit working solutions per type rather than by
installment, so that at least all added functions are actually used
somewhere, but it does not matter much, really.

I was unsure that having int128 implies uint128 availability, so I did not
use it.

The checking extension is funny, but ISTM that these checks should be (are
already?) included in some standard sql test, which will test the macros
from direct SQL operations:

fabien=# SELECT INT2 '1234512434343';
ERROR: value "1234512434343" is out of range for type smallint

Well, a quick look at "src/test/regress/sql/int2.sql" suggests that
there the existing tests should be extended… :-(

--
Fabien.

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Kuntal Ghosh 2019-08-30 08:27:00 Re: POC: Cleaning up orphaned files using undo logs
Previous Message Michael Paquier 2019-08-30 07:34:23 Re: refactoring - share str2*int64 functions