On Tue, Jul 16, 2019 at 01:04:38PM -0700, Andres Freund wrote:
> There is the issue that there already is pg_strtoint16 and
> pg_strtoint32, which do not have the option to not raise an error. I'd
> probably name the non-error throwing ones something like pg_strtointNN_e
> (for extended, or error handling), and have pg_strtointNN wrappers that
> just handle the errors, or reverse the naming (which'd cause a bit of
> churn, but not that much).
>
> That'd also make the code for pg_strtointNN a bit nicer, because we'd
> not need the gotos anymore, they're just there to avoid redundant error
> messages - which'd not be an issue if the error handling were just a
> switch in a separate function. E.g.
Agreed on that. I am wondering if we should use a common wrapper for
all the internal functions taking in input a set of bits16 flags to
control its behavior and put all that into common/script.c:
- Issue an error.
- Check for signedness.
- Base length: 16, 32 or 64.
This would have the advantage to move the error string generation, the
trailing whitespace check, sign handling and such in a single place.
We could have the internal routine return uint64 which is casted
afterwards to a proper result depending on what we use. (Perhaps
that's what you actually meant?)
I would also rather not touch the strtol wrappers that we have able to
handle the base. There is nothing in the tree using it, but likely
there are extensions relying on it. Switching all the existing
callers in the tree to the new routines sounds good to me of course.
Consolidating all that still needs more work, so for now I am
switching the patch as waiting on author.
--
Michael