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