Re: refactoring - share str2*int64 functions

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
Date: 2019-09-16 10:18:24
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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.


Attachment Content-Type Size
str2int-15.patch text/x-diff 39.2 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
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