Skip to content
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.

Sign up for GitHub

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

Jump to bottom

gh-104399: Use newer libtommath APIs when necessary #104407

Merged
merged 4 commits into from
Jun 6, 2023
Merged

gh-104399: Use newer libtommath APIs when necessary #104407

merged 4 commits into from
Jun 6, 2023

Conversation

chrstphrchvz
Copy link
Contributor

@chrstphrchvz chrstphrchvz commented May 11, 2023 •
edited by bedevere-bot
Loading

@bedevere-bot bedevere-bot mentioned this pull request May 11, 2023
Closed
@chrstphrchvz
Copy link
Contributor Author

As with #103842, this requires knowing whether Tcl was built with TCL_WITH_EXTERNAL_TOMMATH defined, which is something not currently available via pkg-config.

@gpshead
Copy link
Member

gpshead commented May 13, 2023

general thought: We could use a buildbot with bleeding edge versions of tcl/tk

serhiy-storchaka
serhiy-storchaka reviewed May 31, 2023
View reviewed changes
Modules/_tkinter.c Outdated
@@ -65,6 +65,12 @@ Copyright (C) 1994 Steen Lumholt.
#endif
#include <tclTomMath.h>

#if defined(TCL_WITH_EXTERNAL_TOMMATH) || (TCL_MAJOR_VERSION >= 9)
Copy link
Member

Choose a reason for hiding this comment

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

The new API is available since 8.6.10. Would not be better to use it as soon as it is available? Potentially it can support larger integers.

Suggested change
#if defined(TCL_WITH_EXTERNAL_TOMMATH) || (TCL_MAJOR_VERSION >= 9)
#if defined(TCL_WITH_EXTERNAL_TOMMATH) || (TK_HEX_VERSION >= 0x0806020a)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not familiar with libtommath APIs nor aware what the exact limits are, but I would think that Tcl is more likely to be the limiting factor anyway, as even the deprecated libtommath functions can handle numbers exceeding what Tcl < 9 can usefully convert to and from (due to various 2**31-1 size limitations on strings, etc.).

Given how Tkinter requires the Tcl version but not the patchlevel to match at compile time and runtime (#104399 (comment)), it should be fine to enable the new functions on Tcl 8.7; this would also allow not having to check for TCL_WITH_EXTERNAL_TOMMATH in this case (although it still has to be checked for #103842).

Copy link
Member

Choose a reason for hiding this comment

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

Agree.

serhiy-storchaka
serhiy-storchaka reviewed Jun 2, 2023
View reviewed changes
Modules/_tkinter.c Outdated Show resolved Hide resolved
@serhiy-storchaka serhiy-storchaka added needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Jun 2, 2023
@serhiy-storchaka serhiy-storchaka merged commit 00d73ca into python:main Jun 6, 2023
@miss-islington
Copy link
Contributor

Thanks @chrstphrchvz for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 6, 2023
@chrstphrchvz @miss-islington
pythongh-104399: Use newer libtommath APIs when necessary (pythonGH-1…
be66e5e 
…04407)

(cherry picked from commit 00d73ca)

Co-authored-by: Christopher Chavez <[email protected]>
@bedevere-bot
Copy link

GH-105343 is a backport of this pull request to the 3.12 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.12 bug and security fixes label Jun 6, 2023
@bedevere-bot
Copy link

GH-105344 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Jun 6, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 6, 2023
@chrstphrchvz @miss-islington
pythongh-104399: Use newer libtommath APIs when necessary (pythonGH-1…
6af5109 
…04407)

(cherry picked from commit 00d73ca)

Co-authored-by: Christopher Chavez <[email protected]>
terryjreedy pushed a commit that referenced this pull request Jun 6, 2023
@miss-islington @chrstphrchvz
[3.12] gh-104399: Use newer libtommath APIs when necessary (GH-104407) (
27cbeb0 
#105343)

gh-104399: Use newer libtommath APIs when necessary (GH-104407)
(cherry picked from commit 00d73ca)

Co-authored-by: Christopher Chavez <[email protected]>
terryjreedy pushed a commit that referenced this pull request Jun 6, 2023
@miss-islington @chrstphrchvz
[3.11] gh-104399: Use newer libtommath APIs when necessary (GH-104407) (
c644fe4 
#105344)

gh-104399: Use newer libtommath APIs when necessary (GH-104407)
(cherry picked from commit 00d73ca)

Co-authored-by: Christopher Chavez <[email protected]>
@chrstphrchvz chrstphrchvz deleted the patch-104399 branch June 9, 2023 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@chrstphrchvz @gpshead @miss-islington @bedevere-bot @serhiy-storchaka