-
-
Notifications
You must be signed in to change notification settings - Fork 31.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-119613: use C99+ functions instead of Py_IS_NAN/INFINITY/FINITE #119619
Conversation
skirpichev
commented
May 27, 2024
•
edited by bedevere-app
bot
Loading
edited by bedevere-app
bot
- Issue: Deprecate Py_IS_NAN/INFINITY/FINITE? #119613
This comment was marked as resolved.
This comment was marked as resolved.
@tim-one is there an on-going reason to have these portability macros? Are NaN values guaranteed to be supported? |
Since 194a952 these macros are just aliases for isnan, isinf, etc: Lines 30 to 40 in 3e8b609
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Simple text replacement.
Python 3.10 and older supported platforms without IEEE 754, without NaN, and without C99 #ifndef Py_IS_NAN
#if defined HAVE_DECL_ISNAN && HAVE_DECL_ISNAN == 1
#define Py_IS_NAN(X) isnan(X)
#else
#define Py_IS_NAN(X) ((X) != (X))
#endif
#endif Python 3.11 and newer requires IEEE 754, NaN and C99 #define Py_IS_NAN(X) isnan(X) Issues:
See also this LWN article: CPython, C standards, and IEEE 754. |
@mdickinson: Are you ok with this change? You wrote a comment about these macros there: #119457 (comment) |
Yes, absolutely. I'm fairly sure the macros were born out of (now somewhat ancient) portability needs that no longer apply, now that C99 (and later) adoption is sufficiently widespread. I'd suggest running all buildbots on this PR, just to be on the safe side. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
🤖 New build scheduled with the buildbot fleet by @mdickinson for commit 13dcd12 🤖 If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
There are tests failures, but hardly it's related to the pr. Something expected, given there are not build bots for non-IEEE-754 platforms, right? |
Agreed - the failures look unrelated. |
Merged, thanks. |