|From:||Michael Paquier <michael(at)paquier(dot)xyz>|
|To:||Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>|
|Cc:||Andres Freund <andres(at)anarazel(dot)de>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org>|
|Subject:||Re: refactoring - share str2*int64 functions|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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.
> 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.
|Next Message||Tomas Vondra||2019-09-16 10:32:44||Re: [PATCH] Incremental sort (was: PoC: Partial sort)|
|Previous Message||Amit Kapila||2019-09-16 08:31:21||Re: block-level incremental backup|