Re: refactoring - share str2*int64 functions

On Sat, Sep 14, 2019 at 10:24:10AM +0200, Fabien COELHO wrote:
> It should rather call pg_strtoint16? And possibly switch the "short int"
> declaration to int16?

Sure, but you get into other problems if using the 16-bit version for
some other fields, which is why it seems to me that we should add an
extra routine for shorts. So for now I prefer discarding this part.

> I do not think that "pg_strtoint_error" should be inlinable. The function is
> unlikely to be called, so it is not performance critical to inline it, and
> would enlarge the executable needlessly. However, the "pg_strto*_check"
> variants should be inlinable, as you have done.

Makes sense.

> About the code, on several instances of:
>
> /* skip leading spaces */
> while (likely(*ptr) && isspace((unsigned char) *ptr)) …
>
> I would drop the "likely(*ptr)".

Right as well. There were two places out of six with that pattern.

> And on several instances of:
>
> !unlikely(isdigit((unsigned char) *ptr)))
>
> ISTM that we want "unlikely(!isdigit((unsigned char) *ptr)))". Parsing
> !unlikely leads to false conclusion and a headache:-)

That part was actually inconsistent with the rest.

> Otherwise, this batch of changes looks ok to me.

Thanks.
--
Michael