Skip to content

gh-132983: Clean-ups for _zstd #133670

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.

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

Merged
merged 16 commits into from
May 9, 2025
Merged

gh-132983: Clean-ups for _zstd #133670

merged 16 commits into from
May 9, 2025

Conversation

AA-Turner
Copy link
Member

@AA-Turner AA-Turner commented May 8, 2025

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

@Rogdham
Copy link
Contributor

Rogdham commented May 8, 2025

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
Copy link
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

@AA-Turner AA-Turner enabled auto-merge (squash) May 8, 2025 18:16
@AA-Turner AA-Turner disabled auto-merge May 8, 2025 19:20
Copy link
Member

@emmatyping emmatyping left a 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!

@AA-Turner AA-Turner merged commit c2a5d4b into python:main May 9, 2025
42 checks passed
@miss-islington-app
Copy link

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
(cherry picked from commit c2a5d4b)

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
@bedevere-app
Copy link

bedevere-app bot commented May 9, 2025

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

@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
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants