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