On 18 February 2016 at 02:00, Vitaly Burovoy <vitaly.burovoy@gmail.com> wrote:
>> + else
>> + have_digits = false;
> Does it worth to move it to the declaration and remove that "else" block?
> + bool have_digits = false;
OK, that's probably a bit neater.
> Is it important to use:
>> + ObjectIdGetDatum(InvalidOid),
>> + Int32GetDatum(-1)));
> instead of "0, -1"? Previously I left immediate constants because I
> found the same thing in jsonb.c (rows 363, 774)...
I think it's preferable to use the macros because they make it clearer
what datatypes are being passed around. I think that's the more common
style, but the JSONB code is not technically wrong either.
>> + if (pg_strcasecmp(strptr, "bytes") == 0)
>> + else if
>> + else if
>> + else if
> It seems little ugly for me. In that case (not using values from GUC)
> I'd create a static array for it and do a small cycle via it. Would it
> better?
That's a matter of personal preference. I prefer the values inline
because then the values are closer to where they're being used, which
for me makes it easier to follow (see e.g., convert_priv_string()).
In guc.c they're in lookup tables because they're referred to from
multiple places, and because it needs to switch between tables based
on context, neither of which is true here. If there is a need to
re-use these values elsewhere in the future it can be refactored, but
right now that feels like an over-engineered solution for this
problem.
>> + NumericGetDatum(mul_num),
> Two more space for indentation.
pgindent pulled that back.
> Also I've found that with allowing exponent there is a weird result
> (we tried to avoid "type numeric" in the error message):
> SELECT pg_size_bytes('123e100000000000000000000000000000000000000000000000000000000000000000000000
> kB');
> ERROR: invalid input syntax for type numeric:
> "123e100000000000000000000000000000000000000000000000000000000000000000000000"
OK, I'll add a check for that.
Thanks for testing.
Regards,
Dean