Re: Proposal: Trigonometric functions in degrees - Mailing list pgsql-hackers

On Mon, Jan 18, 2016 at 5:01 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On 30 November 2015 at 14:11, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> FWIW, I think that probably the best course of action is to go ahead
>> and install POSIX-compliant error checking in the existing functions
>> too.  POSIX:2008 is quite clear about this:
>>
>> : An application wishing to check for error situations should set errno to
>> : zero and call feclearexcept(FE_ALL_EXCEPT) before calling these
>> : functions. On return, if errno is non-zero or fetestexcept(FE_INVALID |
>> : FE_DIVBYZERO | FE_OVERFLOW | FE_UNDERFLOW) is non-zero, an error has
>> : occurred.
>>
>
> Looking at this again, I think it makes sense to make the Inf/NaN
> handling of these functions platform-independent. POSIX.1-2008 is
> pretty explicit about how they ought to behave, which is different
> from the current behaviour on either Linux or Windows:
>
> sin(Inf):
>   POSIX: domain error
>   Linux: NaN
>   Windows: ERROR:  input is out of range

OSX: NaN

> asin(Inf):
>   POSIX: domain error
>   Linux: ERROR:  input is out of range
>   Windows: ERROR:  input is out of range

OSX: NaN

> sin(NaN):
>   POSIX: NaN
>   Linux: NaN
>   Windows: ERROR:  input is out of range

OSX: NaN

> asin(NaN):
>   POSIX: NaN
>   Linux: NaN
>   Windows: ERROR:  input is out of range

OSX: NaN.

> I tried using feclearexcept + fetestexcept as recommended by
> POSIX.1-2008, and that does indeed make the above examples compliant
> on my linux box. Unfortunately it introduces new errors -- asin starts
> generating FE_UNDERFLOW errors for numbers that are really not that
> small, such as asin(1e-9), although the function still returns the
> correct answer. A bigger problem though is that these are C99
> functions and so won't necessarily be available on all supported
> platforms.
>
> So I think that a simpler answer is to just to add explicit tests for
> these inputs and avoid relying on errno, at least for the inverse
> functions, which have pretty clear constraints on their allowed
> inputs. For the forward functions, I'm not sure if there are some
> platforms on which large but finite inputs might generate errors, so I
> think it's safest to keep the existing errno checks as well just in
> case.

Thanks for this investigation. That's really inconsistent...

> Attached are patches for this and the new functions in degrees, now
> with POSIX compatible Inf/NaN handling.

The first patch looks fine to me, a nitpick:
+       /* Be sure to throw an error if the input is infinite --- see dcos */
s/dcos/docs

For patch 2, another nitpick :)
+       return ( sin(x * (M_PI / 180.0)) / sin(30.0 * (M_PI / 180.0)) ) / 2.0;
parenthesis format looks incorrect, too many spaces at the border.

Except those things the second patch looks good to me as well. Let's
have a committer look at it.
-- 
Michael