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
signature.asc
Description: PGP signature
