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 — gh-132983: Clean-ups for `_zstd` by AA-Turner · Pull Request #133670 · python/cpython

Conversation

@AA-Turner

@AA-Turner AA-Turner commented May 8, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

cc @Rogdham (can't request review).

Best reviewed commit-by-commit.

This contains several clean-ups for the _zstd module.

  • _compressionLevel_values is replaced with a single int constant, the default compression level
  • zstd_version_info is initialised from the macros, removing version decomposition arithmetic
  • remove the unused _ZSTD_CStreamSizes
  • Replace _ZSTD_DStreamSizes with ZSTD_DStreamOutSize
  • Remove unused _ZSTD_Config
  • Remove zstd version string from error messages -- it is inconsistently used, and the version can be found from the module-level variables
  • small refactoring of set_zstd_error()
  • Inline the add_parameters function and use the existing PyModule_AddIntMacro rather than defining a local equivalent
  • Remove the leading underscore from several constants in an already private module, use an explicit list of names for the re-exporting in compression.zstd

A

  • Issue: Implement PEP 784 - Adding Zstandard to the Python standard library #132983

Sorry, something went wrong.

Uh oh!

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

All reactions
AA-Turner added 2 commits May 8, 2025 14:42
@AA-Turner
zstd: replace star import with explicit names
0c3c667
@AA-Turner
zstd: factor common parts of set_zstd_error()
e02f66c
@AA-Turner
AA-Turner requested a review from emmatyping May 8, 2025 14:16
@AA-Turner AA-Turner added skip news needs backport to 3.14 bugs and security fixes labels May 8, 2025
@bedevere-app bedevere-app Bot mentioned this pull request May 8, 2025
Implement PEP 784 - Adding Zstandard to the Python standard library #132983
Closed
13 tasks done
@bedevere-app bedevere-app Bot added the awaiting core review label May 8, 2025
AA-Turner added 6 commits May 8, 2025 15:57
@AA-Turner
zstd: Remove ZSTD_versionString from error messages
686ae61
@AA-Turner
zstd: Simplify version_info setup
7ea1ac1
@AA-Turner
zstd: Replace _compressionLevel_values with ZSTD_CLEVEL_DEFAULT
9774338
@AA-Turner
zstd: Remove _ZSTD_CStreamSizes, replace _ZSTD_DStreamSizes with ZSTD…
d1dea98
…_DStreamOutSize
@AA-Turner
zstd: Remove _ZSTD_CONFIG
ac1e39c
@AA-Turner
zstd: Replace add_parameters and ADD_INT_PREFIX_MACRO with PyModule_A…
1a54bf3
…ddIntMacro
@AA-Turner
AA-Turner force-pushed the zstd-clean branch from ca7fa9c to 1a54bf3 Compare May 8, 2025 14:58
@Rogdham

Rogdham commented May 8, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

I read through your commits and have no objections, but I don't feel confident enough to review this on my own.

Did you decide to leave functions of _zstd module with a leading underscore on purpose? (_train_dict/_finalize_dict/_get_param_bounds/_get_frame_info/_set_parameter_types) I have no opinion on it, I'm just curious.

All reactions

Sorry, something went wrong.

Uh oh!

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

@AA-Turner

AA-Turner commented May 8, 2025

Copy link
Copy Markdown
Member Author

Good question -- this PR was getting too big, so I just submitted the changes to the module-level constants. I intend to go through the functions in a follow-up.

A

Rogdham and emmatyping reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

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

@AA-Turner
Merge branch 'main' into zstd-clean
9cb7a3d
@AA-Turner
AA-Turner enabled auto-merge (squash) May 8, 2025 18:16
serhiy-storchaka
serhiy-storchaka reviewed May 8, 2025
View reviewed changes
Comment thread Modules/_zstd/_zstdmodule.c Resolved

Uh oh!

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

Comment thread Modules/_zstd/_zstdmodule.c Resolved

Uh oh!

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

Comment thread Modules/_zstd/_zstdmodule.c Resolved

Uh oh!

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

Comment thread Modules/_zstd/_zstdmodule.c Resolved

Uh oh!

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

@AA-Turner
AA-Turner disabled auto-merge May 8, 2025 19:20
emmatyping
emmatyping previously requested changes May 8, 2025
View reviewed changes

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

I have one request but otherwise I think these changes look like great clean ups. Thanks!

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Modules/_zstd/_zstdmodule.c Resolved

Uh oh!

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

@AA-Turner
Revert "zstd: factor common parts of set_zstd_error()"
663e666
This reverts commit e02f66c.
Rogdham
Rogdham reviewed May 9, 2025
View reviewed changes
Comment thread Modules/_zstd/_zstdmodule.c Resolved

Uh oh!

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

AA-Turner added 4 commits May 9, 2025 11:54
@AA-Turner
Use PyModule_Add
dd1380d
@AA-Turner
Check failure cases for int macros
fcd6561
@AA-Turner
Remove ERR_SET_PLEDGED_INPUT_SIZE
06a4723
@AA-Turner
Use PyLong_FromSize_t
ef2634d
serhiy-storchaka
serhiy-storchaka reviewed May 9, 2025
View reviewed changes
Comment thread Modules/_zstd/_zstdmodule.c Resolved

Uh oh!

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

@AA-Turner
Use runtime version
ff28bf2
@AA-Turner
AA-Turner dismissed emmatyping’s stale review May 9, 2025 12:51

resolved

@bedevere-app bedevere-app Bot added awaiting review and removed awaiting core review labels May 9, 2025
@AA-Turner
AA-Turner requested a review from serhiy-storchaka May 9, 2025 12:58
@AA-Turner
Calculate zstd_version_info in Python
3d3dbf7
picnixz
picnixz reviewed May 9, 2025
View reviewed changes
Comment thread Modules/_zstd/_zstdmodule.c Resolved

Uh oh!

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

Comment thread Modules/_zstd/_zstdmodule.c Resolved

Uh oh!

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

picnixz
picnixz approved these changes May 9, 2025
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels May 9, 2025
@AA-Turner
AA-Turner merged commit c2a5d4b into python:main May 9, 2025
@miss-islington-app

miss-islington-app Bot commented May 9, 2025

Copy link
Copy Markdown

Thanks @AA-Turner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

All reactions

Sorry, something went wrong.

Uh oh!

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

@AA-Turner
AA-Turner deleted the zstd-clean branch May 9, 2025 14:08
@bedevere-app bedevere-app Bot removed the awaiting merge label May 9, 2025
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 9, 2025
@AA-Turner @miss-islington
pythongh-132983: Clean-ups for _zstd (pythonGH-133670)
cba6af2
(cherry picked from commit c2a5d4b)

Co-authored-by: Adam Turner <[email protected]>
@bedevere-app

bedevere-app Bot commented May 9, 2025

Copy link
Copy Markdown

GH-133756 is a backport of this pull request to the 3.14 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-app bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label May 9, 2025
AA-Turner added a commit that referenced this pull request May 9, 2025
@miss-islington @AA-Turner
[3.14] gh-132983: Clean-ups for _zstd (GH-133670) (#133756)
2df021d
gh-132983: Clean-ups for ``_zstd`` (GH-133670)
(cherry picked from commit c2a5d4b)

Co-authored-by: Adam Turner <[email protected]>
@AA-Turner AA-Turner mentioned this pull request May 9, 2025
gh-132983: Simplify _zstd_exec() #133775
Merged
Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request Jul 12, 2025
@AA-Turner @Pranjal095
pythongh-132983: Clean-ups for _zstd (python#133670)
62b28db
taegyunkim pushed a commit to taegyunkim/cpython that referenced this pull request Aug 4, 2025
@AA-Turner @taegyunkim
pythongh-132983: Clean-ups for _zstd (python#133670)
b3ef1c4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@picnixz picnixz picnixz approved these changes
@emmatyping emmatyping emmatyping left review comments
@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka
+2 more reviewers
@Rogdham Rogdham Rogdham left review comments
@aouissatbrahim062-ops aouissatbrahim062-ops aouissatbrahim062-ops left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

skip news

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

@AA-Turner @Rogdham @serhiy-storchaka @emmatyping @picnixz @aouissatbrahim062-ops
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.