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: Simplify `_zstd_exec()` by AA-Turner · Pull Request #133775 · python/cpython

Conversation

@AA-Turner

@AA-Turner AA-Turner commented May 9, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

cc @Rogdham (can't request review).

Follows on from #133670. Again best reviewed commit-by-commit. We remove around 100 lines of C code overall.

The last commit (6a3a135 (#133775)) only serves to merge two functions and contains no behaviour change, but does increase the diff size.

  • Rename type specs to add underscores after 'zstd'
  • Replace the add_type_to_module() function with an ADD_TYPE macro, using PyModule_AddType instead of PyModule_AddObjectRef.
  • Remove the unused 'empty_readonly_memoryview', 'str_flush', 'str_read', 'str_readinto', and 'str_write' module state members
  • Set the CONTINUE/FLUSH_* constants in Python rather than C. To achieve this, export the three ZSTD_EndDirective enum members from the _zstd module.
  • 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 4 commits May 9, 2025 17:10
@AA-Turner
Replace add_type_to_module with ADD_TYPE
fa9b12b
@AA-Turner
Remove unused variables from module state
daf2e31
@AA-Turner
Add constants to ZstdCompressor in Python
55f876a
@AA-Turner
Merge add_vars_to_module into _zstd_exec
6a3a135
@AA-Turner
AA-Turner requested a review from emmatyping May 9, 2025 16:29
@AA-Turner AA-Turner added the skip news label May 9, 2025
@bedevere-app bedevere-app Bot mentioned this pull request May 9, 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 9, 2025
@AA-Turner
PEP 7
d5ff6d3
@AA-Turner
AA-Turner requested review from picnixz and serhiy-storchaka May 9, 2025 16:32
picnixz
picnixz reviewed May 9, 2025
View reviewed changes

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

Overall, I think it's a nice improvement but usually, C classes are read-only (e.g., _sha2.SHA2Type does not allow setting class variables afterwards; the same holds for lzma.LZMACompressor so it would be better to do the same for the Zstd* types)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Modules/_zstd/_zstdmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/compression/zstd/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka reviewed May 9, 2025
View reviewed changes

@serhiy-storchaka serhiy-storchaka 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.

It would be better to make classes implemented in C immutable.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
AA-Turner and emmatyping reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@AA-Turner
Add constants to ZstdCompressor in C
9f402d7
emmatyping
emmatyping approved these changes May 9, 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.

Thanks! These are great refactors.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
picnixz
picnixz reviewed May 9, 2025
View reviewed changes

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

You can add Py_TPFLAGS_IMMUTABLETYPE in this PR or in another.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_zstd/compressor.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 core review labels May 9, 2025
@AA-Turner
AA-Turner merged commit bbe9c31 into python:main May 9, 2025
@bedevere-app bedevere-app Bot removed the awaiting merge label May 9, 2025
@AA-Turner
AA-Turner deleted the zstd-exec branch May 9, 2025 19:15
@AA-Turner AA-Turner added the needs backport to 3.14 bugs and security fixes label 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.
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 9, 2025
@AA-Turner @miss-islington
pythongh-132983: Simplify _zstd_exec() (pythonGH-133775)
d15236e 
(cherry picked from commit bbe9c31)

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

bedevere-app Bot commented May 9, 2025

Copy link
Copy Markdown

GH-133786 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: Simplify _zstd_exec() (GH-133775) (#133786)
5796e3b 
gh-132983: Simplify ``_zstd_exec()`` (GH-133775)
(cherry picked from commit bbe9c31)

Co-authored-by: Adam Turner <[email protected]>
Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request Jul 12, 2025
@AA-Turner @Pranjal095
pythongh-132983: Simplify _zstd_exec() (python#133775)
7f6dc2b
taegyunkim pushed a commit to taegyunkim/cpython that referenced this pull request Aug 4, 2025
@AA-Turner @taegyunkim
pythongh-132983: Simplify _zstd_exec() (python#133775)
db9c2c5
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
@emmatyping emmatyping emmatyping approved these changes
@picnixz picnixz picnixz approved these changes

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.

4 participants

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