On Fri, Aug 30, 2019 at 10:06:11AM +0200, Fabien COELHO wrote:
> Patch applies cleanly, compiles, "make check" ok, but the added functions
> are not used (yet).
Thanks.
> 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.
Yes, the division would be worse than the other. What do you think
about using the previous module I sent and measure how long it takes
to evaluate the overflows in some specific cases N times in loops?
> Maybe I would add a comment before each new section telling about the type,
> eg:
>
> /*
> * UINT16
> */
> add/sub/mul uint16 functions.
Let's do as you suggest here.
> 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 prefer by section, with testing dedicated to each part properly
done so as we can move to the next parts.
> I was unsure that having int128 implies uint128 availability, so I did not
> use it.
The recent Ryu-floating point work has begun using them (see f2s.c and
d2s.c).
> 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:
Sure. But not for the unsigned part as there are no equivalent
in-core data types, still it is possible to trick things with signed
integer arguments. I found my toy useful to check test all
implementations consistently.
> 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… :-(
We can tackle that separately. -32768 is perfectly legal for
smallint, and the test is wrong here.
--
Michael