Re: Efficient output for integer types

On Sat, Sep 21, 2019 at 07:29:25AM +0100, Andrew Gierth wrote:
> >>>>> "David" == David Fetter <david@fetter.org> writes:
> 
>  David> +static inline uint32
>  David> +decimalLength64(const uint64_t v)
> 
> Should be uint64, not uint64_t.

Fixed.

> Also return an int, not a uint32.

Fixed.

> For int vs. int32, my own inclination is to use "int" where the value is
> just a (smallish) number, especially one that will be used as an index
> or loop count, and use "int32" when it actually matters that it's 32
> bits rather than some other size. Other opinions may differ.

Done with int.

>  David> +{
>  David> +    uint32            t;
>  David> +    static uint64_t    PowersOfTen[] = {
> 
> uint64 not uint64_t here too.

Fixed.

>  David> +int32
>  David> +pg_ltoa_n(uint32 value, char *a)
> 
> If this is going to handle only unsigned values, it should probably be
> named pg_ultoa_n.

It does signed values now.

>  David> +    uint32    i = 0, adjust = 0;
> 
> "adjust" is not assigned anywhere else. Presumably that's from previous
> handling of negative numbers?

It was, and now it's gone.

>  David> +        memcpy(a, "0", 1);
> 
>  *a = '0';  would suffice.

Fixed.

>  David> +    i += adjust;
> 
> Superfluous?

Yep. Gone.

>  David> +    uint32_t    uvalue = (uint32_t)value;
> 
> uint32 not uint32_t.

Fixed.

>  David> +    int32        len;
> 
> See above re. int vs. int32.

Done that way.

>  David> +        uvalue = (uint32_t)0 - (uint32_t)value;
> 
> Should be uint32 not uint32_t again.

Done.

> For anyone wondering, I suggested this to David in place of the ugly
> special casing of INT32_MIN. This method avoids the UB of doing (-value)
> where value==INT32_MIN, and is nevertheless required to produce the
> correct result:
> 
> 1. If value < 0, then ((uint32)value) is (value + UINT32_MAX + 1)
> 2. (uint32)0 - (uint32)value
>       becomes  (UINT32_MAX+1)-(value+UINT32_MAX+1)
>       which is (-value) as required
> 
>  David> +int32
>  David> +pg_lltoa_n(uint64_t value, char *a)
> 
> Again, if this is doing unsigned, then it should be named pg_ulltoa_n

Renamed to allow the uint64s that de-special-casing INT32_MIN/INT64_MIN requires.

>  David> +        if (value == PG_INT32_MIN)
> 
> This being inconsistent with the others is not nice.

Fixed.

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate