Skip to content

gh-132983: Clean-ups for _zstd - #133670

Merged
AA-Turner merged 16 commits into
python:mainfrom
AA-Turner:zstd-clean
May 9, 2025
Merged

gh-132983: Clean-ups for _zstd#133670
AA-Turner merged 16 commits into
python:mainfrom
AA-Turner:zstd-clean

Conversation

@AA-Turner

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

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

@AA-Turner
AA-Turner requested a review from emmatypingMay 8, 2025 14:16
@bedevere-appbedevere-appBot mentioned this pull request May 8, 2025
Closed
13 tasks
@Rogdham

Rogdham commented May 8, 2025•
edited
Loading

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.

@AA-Turner

AA-Turner commented May 8, 2025

Copy link
Copy Markdown
MemberAuthor

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

@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 threadModules/_zstd/_zstdmodule.c Outdated
Comment threadModules/_zstd/_zstdmodule.c Outdated
Comment threadModules/_zstd/_zstdmodule.c Outdated
Comment threadModules/_zstd/_zstdmodule.c Outdated
@AA-Turner
AA-Turner disabled auto-merge May 8, 2025 19:20
emmatyping
emmatyping previously requested changes May 8, 2025
View reviewed changes

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

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

Comment threadModules/_zstd/_zstdmodule.c
Rogdham
Rogdham reviewed May 9, 2025
View reviewed changes
Comment threadModules/_zstd/_zstdmodule.c Outdated
serhiy-storchaka
serhiy-storchaka reviewed May 9, 2025
View reviewed changes
Comment threadModules/_zstd/_zstdmodule.c Outdated
picnixz
picnixz reviewed May 9, 2025
View reviewed changes
Comment threadModules/_zstd/_zstdmodule.c
Comment threadModules/_zstd/_zstdmodule.c
@AA-Turner
AA-Turner merged commit c2a5d4b into python:mainMay 9, 2025
@miss-islington-app

miss-islington-appBot 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!

@AA-Turner
AA-Turner deleted the zstd-clean branch May 9, 2025 14:08
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 <9087854+AA-Turner@users.noreply.github.com>
@bedevere-app

bedevere-appBot commented May 9, 2025

Copy link
Copy Markdown

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

@bedevere-appbedevere-appBot 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 <9087854+AA-Turner@users.noreply.github.com>
@AA-TurnerAA-Turner mentioned this pull request May 9, 2025
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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@picnixzpicnixzpicnixz approved these changes
@emmatypingemmatypingemmatyping left review comments
@serhiy-storchakaserhiy-storchakaAwaiting requested review from serhiy-storchaka
+2 more reviewers
@RogdhamRogdhamRogdham left review comments
@aouissatbrahim062-opsaouissatbrahim062-opsaouissatbrahim062-ops left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@AA-Turner@Rogdham@serhiy-storchaka@emmatyping@picnixz@aouissatbrahim062-ops