Hi,
On 2019-07-16 16:11:44 +0900, Michael Paquier wrote:
> numutils.c also has pg_strtoint16 and pg_strtoint32, so the locations
> become rather inconsistent with inconsistent APIs for the manipulation
> of int2 and int4 fields, and scanint8 is just a derivative of the same
> logic. We have two categories of routines here:
> - The wrappers on top of strtol and strtoul & co, which are named
> respectively strtoint and pg_strtouint64 with the patch. The naming
> part is inconsistent, and we only handle uint64 and int32. We don't
> need to worry about int64 and uint32 because they are not used?
> That's fine by me, but at least let's have a consistent naming.
Yea, consistent naming seems like a strong requirement
here. Additionally I think we should just provide a consistent set
rather than what's needed just now. That'll just lead to people
inventing their own again down the line.
> Prefixing the functions with pg_* is a better practice in my opinion
> as we will unlikely run into conflicts this way.
+1
> - The str->integer conversion routines, which actually have very
> similar characteristics to the strtol families as they remove trailing
> whitespaces first, check for a sign, etc, except that they work only
> on base 10.
There's afaict neither a caller that needs the base argument at the
moment, nor one in the tree previously. I'd argue for just making
pg_strtouint64's API consistent.
I'd probably also just use the implementation we have for signed
integers (minus the relevant negation and overflow checks, obviously) -
it's a lot faster, and I think there's value in keeping the
implementations in sync.
Greetings,
Andres Freund
