Re: [Patch] Log10 and hyperbolic functions for SQL:2016 compliance

Hi,

Thanks for your time and advice, Tom!

> [ adding_log10_and_hyperbolic_functions_v1.patch ]

No objection to the feature, but

- Why are you using the float4-width library functions (coshf etc)
rather than the float8-width ones (cosh etc)?

Well, I guess the only reason is that I wasn't paying attention enough... I changed for the float8-width library functions.

 

- I wonder whether these library functions exist everywhere.
If they don't, what will we do about it ... throw an error?

I see that SUSv2 requires cosh() and so on, so it's quite possible
that these do exist everywhere we care about.  I'd be okay with
throwing a patch onto the buildfarm to see, and adding an autoconf
test only if the buildfarm is unhappy.  But we should be clear on
what we're going to do about it if that happens.

I think I was too confident because of the "it works on my laptop" syndrome... I don't know how to answer to this point.

 

> I added regression tests for the new functions, but I didn't for log10
> function, assuming that if log function worked, log10 will work too.

Not quite sure I believe that.

Do you mean I should also add a regression test for the new log10 function too ?

 

Actually, what I'd counsel is that you *not* include tests of what
these do with Inf and NaN.  There is no upside to doing so, and lots
of downside if older platforms are squirrely in their behavior, which
is hardly unlikely (cf opossum ...)

I changed the regression tests for hyperbolic functions, so it doesn't test for Inf and NaN.  

You'll find enclosed the new version of the patch.

Cheers,

Lætitia