Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Comments

Fix Issue 18247 - core.stdc.math functions that never set errno should be pure#2045

Merged
dlang-bot merged 1 commit intodlang:masterfrom
n8sh:pure-stdc-math
Feb 7, 2018
Merged

Fix Issue 18247 - core.stdc.math functions that never set errno should be pure#2045
dlang-bot merged 1 commit intodlang:masterfrom
n8sh:pure-stdc-math

Conversation

@n8sh
Copy link
Member

@n8sh n8sh commented Jan 17, 2018

These include:

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @n8sh! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

  • smaller, focused PRs are easier to review than big ones

  • try not to mix up refactoring or style changes with bug fixes or feature enhancements

  • provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close Bugzilla Description
18247 core.stdc.math functions that never set errno should be pure

@dlang-bot dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label Jan 17, 2018
@ibuclaw
Copy link
Member

ibuclaw commented Jan 17, 2018

Are you sure about that? The latter ones seem fine, as they are bit getters/setters.

I don't believe you when you say that cos, sin and tan are pure.

@n8sh
Copy link
Member Author

n8sh commented Jan 17, 2018

You were right to disbelieve me. On macOS 10.12.6 man cos says:

cos(+-infinity) returns a NaN and raises the "invalid" floating-point exception

But on Linux:

Domain error: x is an infinity
errno is set to EDOM (but see BUGS). An invalid floating-point exception (FE_INVALID) is raised.
...
Before version 2.10, the glibc implementation did not set errno to EDOM when a domain error occurred.

Will fix. I'll also check the other functions.

@n8sh n8sh force-pushed the pure-stdc-math branch 2 times, most recently from f8e6643 to 8e0b69d Compare January 17, 2018 17:33
@ibuclaw
Copy link
Member

ibuclaw commented Jan 19, 2018

I still think there are a couple others (tanh, asinh). I'll check against both glibc and gcc how they attribute these functions.

@n8sh
Copy link
Member Author

n8sh commented Jan 29, 2018

Looking at the glibc sources, sysdeps/ieee754/dbl-64/s_sin.c contains #include <errno.h> and __set_errno. In contrast neither of those appear in:

Neither do tanh/asinh/atan have branches in sysdeps/ieee754/k_standard.c which implements errno setting for a variety of functions.

However if tanh, asinh, and/or atan are sticking points I can remove the pure annotation from them.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 30, 2018

@n8sh - I'll use gcc as a reference, math functions that set errno are marked as MATHFN_ERRNO

Now, having gone through the list, I can say that of all functions you originally gave, only one is listed as setting errno:

  • fdim - ✖️

Looks like cos, sin, and tan are fine afterall.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 30, 2018

Looking at the glibc sources, sysdeps/ieee754/dbl-64/s_sin.c contains #include <errno.h> and __set_errno. In contrast neither of those appear in:

If I was to be pedantic, tanh() calls expm1(), which may set errno.

@n8sh
Copy link
Member Author

n8sh commented Jan 30, 2018

OK, added pure back to sin/cos/tan and removed it from fdim.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 30, 2018

Also newlib documentation is a good point of reference too.

@n8sh
Copy link
Member Author

n8sh commented Feb 6, 2018

Rebased for musl.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Attributes Bug Fix Include reference to corresponding bugzilla issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants