Feedback-ID: i131946ab:Fastmail
Message-ID: <068f0344-ccce-c623-3cfe-d09752db158d@enterprisedb.com>
Date: Thu, 24 Nov 2022 09:35:08 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.4.2
Subject: Re: Non-decimal integer literals
Content-Language: en-US
To: David Rowley <dgrowleyml@gmail.com>
Cc: John Naylor <john.naylor@enterprisedb.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
References: <b239564c-cad0-b23e-c57e-166d883cb97d@enterprisedb.com>
 <577603ec-787b-61b8-23a2-1bd0457ed1c7@enterprisedb.com>
 <be7fb835-abf9-2f5a-de5d-ad946a7d42e1@enterprisedb.com>
 <c42cb38a-ac42-dd92-2935-dce3b9806e26@enterprisedb.com>
 <CA+TgmoZv2Q11P1JbF+ziu1b8vsBpcjfPqXsHMR0hJqSTeLP1xQ@mail.gmail.com>
 <bef9c869-e773-f792-fde2-d7555add2879@enterprisedb.com>
 <CA+TgmoZRJzBe6yKeBpQ2auHndeV-bzygSY=gr0A0KixZ2giKNg@mail.gmail.com>
 <2317133.1643155358@sss.pgh.pa.us>
 <97aa22d7-b80f-efa1-f368-3dbf73a15980@enterprisedb.com>
 <CAFBsxsGcgqJ_KO-1=jCUJKK2z5rCh2qsf7=VgNWzTvakLMDyBQ@mail.gmail.com>
 <bc462aff-675b-86e2-46e3-7eaf5ce3e75f@enterprisedb.com>
 <071c40f2-15cf-5db3-8799-dbac041bb881@enterprisedb.com>
 <CAFBsxsG4m9qK=vyLAj2DGo3vZ6AC0vRxA4gWtExH5RGP=sPo_Q@mail.gmail.com>
 <b70b15fc-7f8d-5370-21fc-4ec3c154aef3@enterprisedb.com>
 <791aee01-f488-0706-2694-08c5b57a3bdd@enterprisedb.com>
 <CAApHDvr6m8Sipff_MjqyQ8HLeDA3LB5x9zE1oMBPBLxFwTUKQw@mail.gmail.com>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
In-Reply-To: 
 <CAApHDvr6m8Sipff_MjqyQ8HLeDA3LB5x9zE1oMBPBLxFwTUKQw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/068f0344-ccce-c623-3cfe-d09752db158d%40enterprisedb.com>
Precedence: bulk

On 23.11.22 09:54, David Rowley wrote:
> On Wed, 23 Nov 2022 at 02:37, Peter Eisentraut
> <peter.eisentraut@enterprisedb.com> wrote:
>> Here is a new patch.
> 
> This looks like quite an inefficient way to convert a hex string into an int64:
> 
>          while (*ptr && isxdigit((unsigned char) *ptr))
>          {
>              int8        digit = hexlookup[(unsigned char) *ptr];
> 
>              if (unlikely(pg_mul_s64_overflow(tmp, 16, &tmp)) ||
>                  unlikely(pg_sub_s64_overflow(tmp, digit, &tmp)))
>                  goto out_of_range;
> 
>              ptr++;
>          }
> 
> I wonder if you'd be better off with something like:
> 
>          while (*ptr && isxdigit((unsigned char) *ptr))
>          {
>              if (unlikely(tmp & UINT64CONST(0xF000000000000000)))
>                  goto out_of_range;
> 
>              tmp = (tmp << 4) | hexlookup[(unsigned char) *ptr++];
>          }
> 
> Going by [1], clang will actually use multiplication by 16 to
> implement the former. gcc is better and shifts left by 4, so likely
> won't improve things for gcc.  It seems worth doing it this way for
> anything that does not have HAVE__BUILTIN_OP_OVERFLOW anyway.

My code follows the style used for parsing the decimal integers. 
Keeping that consistent is valuable I think.  I think the proposed 
change makes the code significantly harder to understand.  Also, what 
you are suggesting here would amount to an attempt to make parsing 
hexadecimal integers even faster than parsing decimal integers.  Is that 
useful?