> Hello, sorry to late for the party, but may I comment on this?
Thank you for picking this up again.
> The first patch reconstructs the operators in layers. These
> functions are called very frequently when used. Some function are
> already inlined in float.h but some static functions in float.h
> also can be and are better be inlined. Some of *_internal,
> point_construct, line_calculate_point and so on are the
> candidates.
They are static functions. I though compiler can decide to inline
them. Do you think adding "inline" to the function signatures are
necessary?
> You removed some DirectFunctionCall to the functions within the
> same file but other functions remain in the style,
> ex. poly_center or on_sl. The function called from the former
> seems large enough but the latter function calls a so small
> function that it could be inlined. Would you like to make some
> additional functions use C call (instead of DirectFunctionCall)
> and inlining them?
I tried to minimise my changes to make reviewing easier. I can make
"_internal" functions for the remaining DirectFunctionCall()s, if you
find it necessary.
> This is not a fault of this patch, but some functions like on_pb
> seems missing comment to describe what it is. Would you like to
> add some?
I will add some on the next version.
> In the second patch, the additional include fmgrprotos.h in
> btree_gin.c seems needless.
It must be something I missed on rebase. I will remove it.
> Some float[48] features were macros
> so that they share the same expressions between float4 and
> float8. They still seems sharing perfectly the same expressions
> in float.h. Is there any reason for converting them into typed
> inline functions?
Kevin Grittner suggested using inline functions instead of macros.
They are easier to use compared to macros, and avoid double-evaluation
hazards.
> In float.h, MAXDOUBLEWIDTH is redueced from 500 to 128, but the
> exponent of double is up to 308 so it doesn't seem sufficient. On
> the other hand we won't use non-scientific notation for extremely
> large numbers and it requires (perhaps) up to 26 bytes in the
> case. In the soruce code, most of them uses "%e" and one of them
> uses '%g". %e always takes the format of
> "-1.(17digits)e+308".. So it would be less than 26
> characters.
>
> =# set extra_float_digits to 3;
> =# select -1.221423424320453e308::float8;
> ?column?
> ---------------------------
> -1.22142342432045302e+308
>
> man printf: (linux)
>> Style e is used if the exponent from its conversion is less than
>> -4 or greater than or equal to the precision.
>
> So we should be safe to have a buffer with 26 byte length and 500
> bytes will apparently too large and even 128 will be too loose in
> most cases. So how about something like the following?
>
> #define MINDOUBLEWIDTH 32
Should it be same for float4 and float8?
> ...
> float4out@float.c<modified>:
>> int ndig = FLT_DIG + extra_float_digits;
>>
>> if (ndig < 1)
>> ndig = 1;
>>
>> len = snprintf(ascii, MINDOUBLEWIDTH + 1, "%+.*g", ndig, num);
>> if (len > MINDOUBLEWIDTH + 1)
>> {
>> ascii = (char *) repalloc(ascii, len);
>> if (snprintf(ascii, len, "%+.*e", ndig, num) > len)
>> error(ERROR, "something wrong happens...");
>> }
>
> I don't think the if part can be used so there would be no
> performance degradation, I believe.
Wouldn't this change the output of the float datatypes? That would be
a backwards incompatible change.
> I'd like to pause here.
I will submit new versions after your are done with your review.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
