WebSafe 3.7github.com
|
|
🏠
Skip to content

gh-132983: Clean-ups for _zstd#133670

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

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

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>
Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request Jul 12, 2025
taegyunkim pushed a commit to taegyunkim/cpython that referenced this pull request Aug 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

6 participants

Comments