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?
