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
