GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-119613: deprecate Py_IS_NAN/INFINITY and Py_IS_FINITE by skirpichev · Pull Request #119701 · python/cpython

Conversation

@skirpichev

@skirpichev skirpichev commented May 29, 2024 •
edited by github-actions Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Deprecate Py_IS_NAN/INFINITY/FINITE? #119613

📚 Documentation preview 📚: https://cpython-previews--119701.org.readthedocs.build/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@skirpichev
pythongh-119613: deprecate Py_IS_NAN/INFINITY and Py_IS_FINITE
e7696ba
@bedevere-app bedevere-app Bot added the awaiting review label May 29, 2024
@bedevere-app bedevere-app Bot mentioned this pull request May 29, 2024
Deprecate Py_IS_NAN/INFINITY/FINITE? #119613
Closed
hugovk
hugovk reviewed May 29, 2024
View reviewed changes
Comment thread Doc/whatsnew/3.14.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/whatsnew/3.14.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

skirpichev and others added 3 commits May 29, 2024 06:04
@skirpichev @hugovk
Update Doc/whatsnew/3.14.rst
dd3dec5
Co-authored-by: Hugo van Kemenade <[email protected]>
@skirpichev
Address review: move deprecation news
cee4721
@skirpichev
+ changelog entry
3ecd159
erlend-aasland
erlend-aasland reviewed May 29, 2024
View reviewed changes
Comment thread Doc/whatsnew/3.14.rst Outdated
----------

* Deprecate ``Py_IS_NAN``, ``Py_IS_INFINITY`` and ``Py_IS_FINITE`` macros,
use instead ``isnan``, ``isinf`` and ``isfinite`` available from ``<math.h>``

@erlend-aasland erlend-aasland May 29, 2024

Copy link
Copy Markdown
Contributor

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.

Suggested change
use instead ``isnan``, ``isinf`` and ``isfinite`` available from ``<math.h>``
use instead :c:func:`!isnan`, :c:func:`!isinf` and :c:func:`!isfinite` available from :file:`math.h`.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@skirpichev skirpichev May 29, 2024

Copy link
Copy Markdown
Member Author

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.

Actually, these are usually macros, e.g.:
https://en.cppreference.com/w/c/numeric/math/isinf

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@erlend-aasland erlend-aasland May 29, 2024

Copy link
Copy Markdown
Contributor

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.

Sure; but this is about the markup :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
erlend-aasland
erlend-aasland reviewed May 29, 2024
View reviewed changes
Comment thread Doc/whatsnew/3.14.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

skirpichev
skirpichev commented May 29, 2024
View reviewed changes
Comment thread Doc/whatsnew/3.14.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev @erlend-aasland
Apply suggestions from code review
b5ede50
Co-authored-by: Erlend E. Aasland <[email protected]>
vstinner
vstinner reviewed May 29, 2024
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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.

I would prefer to soft deprecate these macros, with a link to the glossary in thr What's New and Changelog entries (:term: syntax).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@skirpichev
Address review: emphasise soft deprecation
d4008a1
vstinner
vstinner reviewed May 29, 2024
View reviewed changes
Comment thread Include/pymath.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Include/pymath.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

skirpichev and others added 2 commits May 29, 2024 11:14
@skirpichev @vstinner
Update Include/pymath.h
3815323
Co-authored-by: Victor Stinner <[email protected]>
@skirpichev
Address review: comments in Include/pymath.h
10e1bdc
skirpichev
skirpichev commented May 29, 2024
View reviewed changes
Comment thread Include/pymath.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Include/pymath.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Include/pymath.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

vstinner and others added 3 commits May 29, 2024 11:45
@vstinner @skirpichev
Update Include/pymath.h
97da8f2
Co-authored-by: Sergey B Kirpichev <[email protected]>
@vstinner @skirpichev
Update Include/pymath.h
a3c0b44
Co-authored-by: Sergey B Kirpichev <[email protected]>
@vstinner @skirpichev
Update Include/pymath.h
b45447f
Co-authored-by: Sergey B Kirpichev <[email protected]>
vstinner
vstinner approved these changes May 29, 2024
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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. IMO a soft deprecation is enough. Keeping these macros around is not a major maintenance burden.

If PEP 743 is accepted, maybe we can provide a way to remove this function with an opt-in option.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels May 29, 2024
@vstinner
vstinner enabled auto-merge (squash) May 29, 2024 09:48
@vstinner

vstinner commented May 29, 2024

Copy link
Copy Markdown
Member

Thanks for the updates @skirpichev, I enabled automerge.

@skirpichev: If you are interested, there are other macros which might be worth it to soft deprecate such as Py_MEMCPY(). I removed this macro in the limited C API version 3.11:

#if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 < 0x030b0000
#  define Py_MEMCPY memcpy
#endif
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev

skirpichev commented May 29, 2024

Copy link
Copy Markdown
Member Author

@vstinner, I think automerge wont work due to canceled test (I saw similar failure in #118000).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner
vstinner merged commit 0cdc5c8 into python:main May 29, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label May 29, 2024
@skirpichev
skirpichev deleted the deprecate-py-is-nan-p2-119613 branch May 29, 2024 10:52
@skirpichev

skirpichev commented Jun 4, 2024

Copy link
Copy Markdown
Member Author

@vstinner, we can deprecate also Py_INFINITY, Py_NAN and Py_HUGE_VAL. For the last one, probably we could also update code to use INFINITY instead. What do you think?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Jun 4, 2024

Copy link
Copy Markdown
Member

What do you think?

You can start by avoiding these macros in Python code base and see how it goes.

Py_INFINITY and Py_NAN add a (double) cast.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev

skirpichev commented Jun 4, 2024

Copy link
Copy Markdown
Member Author

Py_INFINITY and Py_NAN add a (double) cast.

Hmm, from C11 Sec 7.2: "The macro INFINITY expands to a constant expression of type float representing positive or unsigned infinity, if available [...] The macro NAN is defined if and only if the implementation supports quiet NaNs for the float type. It expands to a constant expression of type float representing a quiet NaN." (c)

As now build requirements include "Support for IEEE 754 floating point numbers and floating point Not-a-Number (NaN).", I think nowadays this type cast is useless.

Yet, anyway, replacing (with a soft deprecation) Py_HUGE_VAL by Py_INFINITY - probably does make sense. I was mentioned in #104202 (comment). One obvious pro: current using both Py_HUGE_VAL and Py_INFINITY may confuse readers.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev skirpichev mentioned this pull request Jun 4, 2024
gh-120026: deprecate (soft) Py_HUGE_VAL macro #120027
Merged
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@skirpichev @hugovk
@erlend-aasland @vstinner @estyxx
pythongh-119613: Soft deprecate Py_IS_NAN/INFINITY/FINITE (python#119701
173db92
)

Co-authored-by: Hugo van Kemenade <[email protected]>
Co-authored-by: Erlend E. Aasland <[email protected]>
Co-authored-by: Victor Stinner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hugovk hugovk hugovk left review comments
@erlend-aasland erlend-aasland erlend-aasland left review comments
@vstinner vstinner vstinner approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@skirpichev @vstinner @hugovk @erlend-aasland
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.