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 — bpo-45548: Remove _math.c workarounds for pre-C99 libm by tiran · Pull Request #29179 · python/cpython

Conversation

@tiran

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

Uh oh!

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

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 [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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@tiran
tiran requested review from brettcannon and mdickinson October 22, 2021 23:33
@tiran tiran assigned mdickinson Oct 22, 2021
@bedevere-bot bedevere-bot added the awaiting core review label Oct 22, 2021
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Oct 22, 2021
@brettcannon

brettcannon commented Oct 23, 2021

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brettcannon brettcannon mentioned this pull request Oct 23, 2021
bpo-45548: Inline _math.c into _math.h #29181
Closed
@tiran
tiran force-pushed the bpo-45548-math branch from 2ad35e9 to 643473f Compare October 23, 2021 08:33
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tiran
tiran force-pushed the bpo-45548-math branch from 643473f to ad013af Compare October 24, 2021 19:59
@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
tiran force-pushed the bpo-45548-math branch from ad013af to ba5de5f Compare October 24, 2021 20:00
@tiran

tiran commented Oct 24, 2021

Copy link
Copy Markdown
Member Author

@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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mdickinson
mdickinson approved these changes Oct 25, 2021
View reviewed changes

@mdickinson mdickinson 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.

Mostly LGTM; two nitpicks and one question.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Misc/NEWS.d/next/Build/2021-10-24-21-49-49.bpo-45548.UWx0UC.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/mathmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread setup.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Oct 25, 2021
tiran and others added 3 commits October 25, 2021 10:58
@tiran @mdickinson
Update Misc/NEWS.d/next/Build/2021-10-24-21-49-49.bpo-45548.UWx0UC.rst
0ea1734 
Co-authored-by: Mark Dickinson <[email protected]>
@tiran
Remove _math.c depends from setup.py
fc3db0c
@tiran
Update comments, it's _math.h now
45b7fa6
@tiran
tiran requested a review from mdickinson October 25, 2021 08:19
@tiran tiran added the 🤖 automerge label Oct 25, 2021
@miss-islington
miss-islington merged commit fa26245 into python:main Oct 25, 2021
@bedevere-bot bedevere-bot removed the awaiting merge label Oct 25, 2021
@brettcannon brettcannon mentioned this pull request Oct 25, 2021
bpo-45548: Have Modules/Setup only try to compile _math once #29177
Closed
@skirpichev skirpichev mentioned this pull request Aug 5, 2024
Merge m_atan2() and c_atan2() helper functions (or replace libm's atan2) #122681
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

Uh oh!

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

Assignees

@mdickinson mdickinson

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.

6 participants

@tiran @brettcannon @mdickinson @the-knights-who-say-ni @bedevere-bot @miss-islington
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.