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: _zstd: use Py_UNREACHABLE in _zstd_load_impl by Rogdham · Pull Request #137320 · python/cpython

Conversation

@Rogdham

@Rogdham Rogdham commented Aug 2, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

There are 2 functions called _zstd_load_impl: one for compression and one for decompression.

In these functions, the type variable is supposed to represent the dict type. Due to the way the function is called, the dict type is always one of DICT_TYPE_DIGESTED/DICT_TYPE_UNDIGESTED/DICT_TYPE_PREFIX. However, the implementations differs in the unreachable case:

  • For compression: call to Py_UNREACHABLE
  • For decompression: SystemError exception

This PR harmonizes the two implementations by using Py_UNREACHABLE.

  • 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
@Rogdham
Zstd: use Py_UNREACHABLE
b302ef8
@bedevere-app bedevere-app Bot added the awaiting review label Aug 2, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Aug 2, 2025
Implement PEP 784 - Adding Zstandard to the Python standard library #132983
Closed
13 tasks done
@picnixz picnixz added needs backport to 3.14 bugs and security fixes skip news labels Aug 2, 2025
@picnixz
picnixz requested a review from emmatyping August 2, 2025 09:13
emmatyping
emmatyping approved these changes Aug 2, 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.

Thank you!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting core review and removed awaiting review labels Aug 2, 2025
ZeroIntensity
ZeroIntensity approved these changes Aug 3, 2025
View reviewed changes

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

LGTM as well.

@emmatyping Would you like to use your new green button?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Aug 3, 2025
AA-Turner
AA-Turner reviewed Aug 3, 2025
View reviewed changes

@AA-Turner AA-Turner left a comment •
edited
Loading

Uh oh!

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

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.

Does it make sense to have a test that we don't hit this unreachable path?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ZeroIntensity

ZeroIntensity commented Aug 4, 2025

Copy link
Copy Markdown
Member

Hm, how are you envisioning that? The whole point of Py_UNREACHABLE here is that no code path can hit it, so there's not really a test we could write to stress that.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AA-Turner

AA-Turner commented Aug 4, 2025

Copy link
Copy Markdown
Member

I'm not a fan of needing logic like this, and it would be nice to exhaustively test that we never hit it, but I haven't had a good idea, no.

A
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AA-Turner

AA-Turner commented Aug 4, 2025

Copy link
Copy Markdown
Member

Anyway, the compression path already uses Py_UNREACHABLE, so likely best to merge for pragmatism.

A
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AA-Turner
AA-Turner merged commit 57eab1b into python:main Aug 4, 2025
@miss-islington-app

miss-islington-app Bot commented Aug 4, 2025

Copy link
Copy Markdown

Thanks @Rogdham for the PR, and @AA-Turner for merging it 🌮🎉.. 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.
@bedevere-app bedevere-app Bot removed the awaiting merge label Aug 4, 2025
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 4, 2025
@Rogdham @miss-islington
pythongh-132983: Use Py_UNREACHABLE in _zstd_load_impl() (pyt…
62f57a6 
…honGH-137320)

(cherry picked from commit 57eab1b)

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

bedevere-app Bot commented Aug 4, 2025

Copy link
Copy Markdown

GH-137360 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 Aug 4, 2025
@Rogdham
Rogdham deleted the zstd_load_impl_pyunreachable branch August 4, 2025 06:27
hugovk pushed a commit that referenced this pull request Aug 7, 2025
@miss-islington @Rogdham
[3.14] gh-132983: Use Py_UNREACHABLE in _zstd_load_impl() (GH…
0fc49be 
…-137320) (#137360)

Co-authored-by: Rogdham <[email protected]>
Agent-Hellboy pushed a commit to Agent-Hellboy/cpython that referenced this pull request Aug 19, 2025
@Rogdham @Agent-Hellboy
pythongh-132983: Use Py_UNREACHABLE in _zstd_load_impl() (pyt…
4f35853 
…hon#137320)
kumaraditya303 pushed a commit to miss-islington/cpython that referenced this pull request Sep 9, 2025
@miss-islington @Rogdham @kumaraditya303
[3.14] pythongh-132983: Use Py_UNREACHABLE in ``_zstd_load_impl()…
301dcde 
…`` (pythonGH-137320) (python#137360)

Co-authored-by: Rogdham <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AA-Turner AA-Turner AA-Turner left review comments
@emmatyping emmatyping emmatyping approved these changes
@ZeroIntensity ZeroIntensity ZeroIntensity 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.

5 participants

@Rogdham @ZeroIntensity @AA-Turner @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.