Michaël,
>> I have taken the liberty to optimize the existing int64 function by removing
>> spurious tests.
>
> Which are?
- *ptr && WHATEVER(*ptr)
*ptr is redundant, WHATEVER yields false on '\0', and it costs on each
char but at the end. It might be debatable in some places, e.g. it is
likely that there are no spaces in the string, but likely that there are
more than one digit.
If you want all/some *ptr added back, no problem.
- isdigit repeated on if and following while, used if/do-while instead.
>> I have not created uint64 specific inlined overflow functions.
>
> Why? There is a comment below ;p
See comment about comment below:-)
>> If it looks ok, a separate patch could address the 32 & 16 versions.
>
> I am surprised to see a negative diff
Is it? Long duplicate functions are factored out (this was my initial
intent), one file is removed…
> actually just by doing that (adding the 32 and 16 parts will add much
> more code of course). At quick glance, I think that this is on the
> right track. Some comments I have on the top of my mind:
> - It would me good to have the unsigned equivalents of
> pg_mul_s64_overflow, etc. These are simple enough,
Hmmm. Have you looked at the fallback cases when the corresponding
builtins are not available?
I'm unsure of a reliable way to detect a generic unsigned int overflow
without expensive dividing back and having to care about zero…
So I was pretty happy with my two discreet, small and efficient tests.
> and per the feedback from Andres they could live in common/int.h.
Could be, however "string.c" already contains a string to int conversion
function, so I put them together. Probably this function should be
removed in the end, though.
> - It is more consistent to use upper-case statuses in the enum
> strtoint_status.
I thought of that, but first enum I found used lower case, so it did not
seem obvious that pg style was to use upper case. Indeed, some enum
constants use upper cases.
> Could it be renamed to pg_strtoint_status?
Sure. I also prefixed the enum constants for consistency.
Attached patch uses a prefix and uppers constants. Waiting for further
input about other comments.
--
Fabien.
