Skip to content

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

Merged
miss-islington merged 4 commits into
python: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 into
python:mainfrom
tiran:bpo-45548-math

Conversation

@tiran

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

Copy link
Copy Markdown
Member

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 mdickinson@enthought.com
Co-authored-by: Brett Cannon brett@python.org
Signed-off-by: Christian Heimes christian@python.org

https://bugs.python.org/issue45548

Automerge-Triggered-By: GH:tiran

@brettcannon

brettcannon commented Oct 23, 2021

Copy link
Copy Markdown
Member

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

@brettcannonbrettcannon mentioned this pull request Oct 23, 2021
Closed
@mdickinson

mdickinson commented Oct 24, 2021

Copy link
Copy Markdown
Member

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

mdickinson commented Oct 24, 2021

Copy link
Copy Markdown
Member

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

mdickinson commented Oct 24, 2021

Copy link
Copy Markdown
Member

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 <mdickinson@enthought.com>
Co-authored-by: Brett Cannon <brett@python.org>
Signed-off-by: Christian Heimes <christian@python.org>
@tirantiran changed the title bpo-45548: Directly include _math.cbpo-45548: Remove _math.c workarounds for pre-C99 libmOct 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 ownerOctober 24, 2021 20:00
@tiran

tiran commented Oct 24, 2021

Copy link
Copy Markdown
MemberAuthor

@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

@mdickinsonmdickinson left a comment

Copy link
Copy Markdown
Member

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.

Comment threadMisc/NEWS.d/next/Build/2021-10-24-21-49-49.bpo-45548.UWx0UC.rst Outdated
Comment threadModules/mathmodule.c Outdated
Comment threadsetup.py Outdated
@tiran
tiran requested a review from mdickinsonOctober 25, 2021 08:19
@miss-islington
miss-islington merged commit fa26245 into python:mainOct 25, 2021
@brettcannonbrettcannon mentioned this pull request Oct 25, 2021
Closed
@skirpichevskirpichev mentioned this pull request Aug 5, 2024
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brettcannonbrettcannonAwaiting requested review from brettcannon
@mdickinsonmdickinsonAwaiting requested review from mdickinson

Assignees

@mdickinsonmdickinson

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