Skip to content

bpo-45440: Require math.h isinf() to build #28894

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

Merged
merged 1 commit into from
Oct 13, 2021
Merged

bpo-45440: Require math.h isinf() to build #28894

merged 1 commit into from
Oct 13, 2021

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Oct 12, 2021

Building Python now requires a C99 <math.h> header file providing
isinf(), isnan() and isfinite() functions.

Remove the Py_FORCE_DOUBLE() macro. It was used by the
Py_IS_INFINITY() macro.

Changes:

  • Remove Py_IS_NAN(), Py_IS_INFINITY() and Py_IS_FINITE()
    in PC/pyconfig.h.
  • Remove the _Py_force_double() function.
  • configure no longer checks if math.h defines isinf(), isnan() and
    isfinite().

https://bugs.python.org/issue45440

@vstinner
Copy link
Member Author

@mdickinson: In 2021, is it now ok to require a C99 math.h header file? :-)

@vstinner
Copy link
Member Author

Py_FORCE_DOUBLE() is not used in the PyPI top 5000 projects:

$ ./search_pypi_top_5000.sh Py_FORCE_DOUBLE
pypi-top-5000_2021-08-17/frozendict-2.0.6.tar.gz

frozendict-2.0.6.tar.gz contains different copies of pymath.h, but it doesn't call Py_FORCE_DOUBLE().

@vstinner
Copy link
Member Author

cc @serhiy-storchaka

@mdickinson
Copy link
Member

In 2021, is it now ok to require a C99 math.h header file? :-)

Yes, provided that MSVC has the functions we need. The practical blocker in the past has been lack of C99 support in MS compilers; other platforms haven't been an issue.

@vstinner
Copy link
Member Author

@mdickinson: Does it mean that you are ok to merge this PR? Can you maybe approve it?

Yes, provided that MSVC has the functions we need. The practical blocker in the past has been lack of C99 support in MS compilers; other platforms haven't been an issue.

My PR stills still avoids C99 with MSC in pymath.h. It uses MSC specific functions:

#  define Py_IS_FINITE(X) _finite(X)
#  define Py_IS_NAN(X) _isnan(X)
static inline double Py_IS_INFINITY(double x) { return (!_finite(x) && !_isnan(x)); }

"Python 3.6 and later can use Microsoft Visual Studio 2017" says https://devguide.python.org/setup/#windows-compiling

Maybe we can also use C99 functions on Windows. I didn't try yet. I would prefer to make this change separately to be able to revert it if it causes issues later.

@vstinner
Copy link
Member Author

Remove the Py_FORCE_DOUBLE() macro.

I'm a little bit worried by this change. That's why I documented it properly. But it's not used in the PyPI top 5000 modules, so it should be fine. We can easily revert this removal if it causes issues.

@mdickinson
Copy link
Member

Does it mean that you are ok to merge this PR? Can you maybe approve it?

Working right now, but I can do a proper review later today (UTC+1), after work.

@mdickinson mdickinson self-requested a review October 12, 2021 08:53
@vstinner
Copy link
Member Author

@mdickinson: I updated my PR to remove the code specific to Windows (MSC). MSC now uses the same code than GCC/clang, Linux, etc. All platforms now call isinf(), isnan() and isfinite() functions.

The code is now even simpler.

@vstinner
Copy link
Member Author

PR rebased to fix a merge conflict.

Building Python now requires a C99 <math.h> header file providing
isinf(), isnan() and isfinite() functions.

Remove the Py_FORCE_DOUBLE() macro. It was used by the
Py_IS_INFINITY() macro.

Changes:

* Remove Py_IS_NAN(), Py_IS_INFINITY() and Py_IS_FINITE()
  in PC/pyconfig.h.
* Remove the _Py_force_double() function.
* configure no longer checks if math.h defines isinf(), isnan() and
  isfinite().
@vstinner vstinner merged commit 194a952 into python:main Oct 13, 2021
@vstinner vstinner deleted the isinf branch October 13, 2021 21:27
@vstinner
Copy link
Member Author

I merged my PR.

Since the first version of this PR, I made some change to remove MSC specific code. The final change is simpler: just use C99 isinf(), isnan() and isfinite() functions.

I now hope that buildbots will like it :-)

@mdickinson
Copy link
Member

Working right now, but I can do a proper review later today (UTC+1), after work.

Sorry; real life got in the way. Thanks for not letting my lack of review block this. I'll take a look post-merge.

@vstinner
Copy link
Member Author

Sorry; real life got in the way. Thanks for not letting my lack of review block this. I'll take a look post-merge.

I love post-merge reviews :-) I'm also open to revert if needed. Don't worry.

I decided to merge my change to see how it goes with all buildbots. So far, so good.

I also like to make such change early during the Python devcycle to have more time to test it before the 3.11.0 final version. So again, we can consider to revert it if needed.

It seems like nowadays, it became possible to rely on C99 features like <math.h> isinf() ;-)

I'm curious to see if we can remove hypot(), copysign() and round() fallback implementations from Python/pymath.c.

@mdickinson
Copy link
Member

I'm curious to see if we can remove hypot(), copysign() and round() fallback implementations from Python/pymath.c.

That seems like a question that should be easy to answer: the only platform that we care about that might not have full support for C99 is Windows. So if we remove all three of the fallbacks, and the buildbots are still happy for Windows versions back as far as the most ancient version of Windows we still care about (Windows 7?) then we're good.

@mdickinson
Copy link
Member

(Windows 7?)

Looks like it's Windows 8.1 for Python >= 3.11.

@vstinner
Copy link
Member Author

That seems like a question that should be easy to answer: the only platform that we care about that might not have full support for C99 is Windows. So if we remove all three of the fallbacks, and the buildbots are still happy for Windows versions back as far as the most ancient version of Windows we still care about (Windows 7?) then we're good.

Well, let's see ;-) I created: #28977

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants