Skip to content

bpo-45548: Remove _math.c workarounds for pre-C99 libm#29179

Merged
miss-islington merged 4 commits intopython:mainfrom
tiran:bpo-45548-math
Oct 25, 2021
Merged

bpo-45548: Remove _math.c workarounds for pre-C99 libm#29179
miss-islington merged 4 commits intopython:mainfrom
tiran:bpo-45548-math

Conversation

@tiran
Copy link
Member

@tiran tiran commented Oct 22, 2021 •
edited by miss-islington
Loading

The :mod:math and :mod:cmath implementation now require a C99 compatible
libm and no longer ship with workarounds for missing acosh, asinh,
expm1, and log1p functions.

The changeset also removes _math.c and moves the last remaining
workaround into _math.h. This simplifies static builds with
Modules/Setup and resolves symbol conflicts.

Co-authored-by: Mark Dickinson [email protected]
Co-authored-by: Brett Cannon [email protected]
Signed-off-by: Christian Heimes [email protected]

https://bugs.python.org/issue45548

Automerge-Triggered-By: GH:tiran

@brettcannon
Copy link
Member

brettcannon commented Oct 23, 2021

I created an alternative approach but just inlining into _math.h: #29181 .

@brettcannon brettcannon mentioned this pull request Oct 23, 2021
Closed
@mdickinson
Copy link
Member

mdickinson commented Oct 24, 2021

I don't see it as a major problem. The functions are small.

Agreed. Looks fine to me. I somewhat prefer @brettcannon's approach (it feels odd to be including a .c file), but either seems fine, and it looks as though #29181 is having some issues with the Windows build.

@mdickinson
Copy link
Member

mdickinson commented Oct 24, 2021

A bit off-topic (and probably fodder for a separate PR), but it looks as though we ought to be able to get rid of most of the functionality in _math.c: if we can assume C99, then we can assume the existence of asinh, atanh, etc.

@mdickinson
Copy link
Member

mdickinson commented Oct 24, 2021

we ought to be able to get rid of most of the functionality in _math.c

Looking more closely, I think we can assume that all of HAVE_ACOSH, HAVE_ASINH, HAVE_ATANH, HAVE_EXPM1 and HAVE_LOG1P are defined. That would make almost all of _math.c redundant, with the sole exception of the extra zero-handling in _Py_log1p. But again, that can be fixed in a separate PR.

@tiran @mdickinson @brettcannon
bpo-45548: Remove _math.c workarounds for pre-C99 libm
ba5de5f 
The :mod:`math` and :mod:`cmath` implementation now require a C99 compatible
``libm`` and no longer ship with workarounds for missing acosh, asinh,
expm1, and log1p functions.

The changeset also removes ``_math.c`` and moves the last remaining
workaround into ``_math.h``. This simplifies static builds with
``Modules/Setup`` and resolves symbol conflicts.

Co-authored-by: Mark Dickinson <[email protected]>
Co-authored-by: Brett Cannon <[email protected]>
Signed-off-by: Christian Heimes <[email protected]>
@tiran tiran changed the title bpo-45548: Directly include _math.c bpo-45548: Remove _math.c workarounds for pre-C99 libm Oct 24, 2021
@tiran tiran marked this pull request as ready for review October 24, 2021 20:00
@tiran tiran requested a review from a team as a code owner October 24, 2021 20:00
@tiran
Copy link
Member Author

tiran commented Oct 24, 2021

@mdickinson I implemented your suggestion, moved the log1p workaround into _math.h and deleted _math.c.

(Sorry for the squashed commit. I made an error while rewriting the commit message)

mdickinson
mdickinson approved these changes Oct 25, 2021
View reviewed changes
Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM; two nitpicks and one question.

@tiran tiran requested a review from mdickinson October 25, 2021 08:19
@miss-islington miss-islington merged commit fa26245 into python:main Oct 25, 2021
@brettcannon brettcannon mentioned this pull request Oct 25, 2021
Closed
@skirpichev skirpichev mentioned this pull request Aug 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brettcannon brettcannon Awaiting requested review from brettcannon

@mdickinson mdickinson Awaiting requested review from mdickinson

Assignees

@mdickinson mdickinson

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@tiran @brettcannon @mdickinson @the-knights-who-say-ni @bedevere-bot @miss-islington