Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-134768: Fix definition ofmt_continue_should_break()#134769

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
gpshead merged 1 commit intopython:mainfromthesamesam:py-assertions-vs-zstd
May 30, 2025

Conversation

thesamesam
Copy link
Contributor

@thesamesamthesamesam commentedMay 27, 2025•
edited by bedevere-appbot
Loading

In121ed71, mt_continue_should_break was changed to be guarded byPy_DEBUG, but it's used incompress_mt_continue_lock_held with justassert, so it needs to be available whenNDEBUG is undefined too.

Py_DEBUG impliesNDEBUG is undefined, so we can check just that.

Fixes:121ed71

In121ed71, mt_continue_should_breakwas changed to be guarded by `Py_DEBUG`, but it's used in `compress_mt_continue_lock_held`with just `assert`, so it needs to be available when `NDEBUG` is undefinedtoo.`Py_DEBUG` implies `NDEBUG` is undefined, so we can check just that.Fixes:121ed71
@emmatyping
Copy link
Member

emmatyping commentedMay 27, 2025•
edited
Loading

Just verifying my understanding, because "NDEBUG" is rather confusingly named it seems: the failure case is you compile in debug (not--enable-optimizations) which definesPy_DEBUG, but also enable assertions via--enable-assertions (which definesNDEBUG)?

@mgorny
Copy link
Contributor

No. The failure is that you compile regular (which doesn't setPy_DEBUG) but also--with-assertions (which implies-UNDEBUG).

emmatyping reacted with thumbs up emoji

@thesamesam
Copy link
ContributorAuthor

thesamesam commentedMay 27, 2025•
edited
Loading

Other way around:

  • --with-pydebug (Py_DEBUG) =>-UNDEBUG -DPy_DEBUG ->-DPy_DEBUG (works on main)
  • --with-assertions (-UNDEBUG) => nothing (needs this PR becauseassert isnot compiled out)
  • nothing (-DNDEBUG) =>-DNDEBUG (works on main, because theassert is compiled out)

It's really stupid and it's to do with howassert is defined. By default,assert does something. IfNDEBUG is defined, itdoesn't. It's very common to setNDEBUG for performance and CPython does thatunless you pass--with-assertions.

Copy link
Member

@emmatypingemmatyping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Okay, makes sense, thanks for the explanations

thesamesam reacted with thumbs up emoji
@mgorny
Copy link
Contributor

Could we merge this, please? It just bit me again as I was about to recheck a failing assertion onmain.

@gpsheadgpsheadenabled auto-merge (squash)May 30, 2025 04:42
@gpsheadgpshead merged commit2f2bee2 intopython:mainMay 30, 2025
52 checks passed
@miss-islington-app
Copy link

Thanks@thesamesam for the PR, and@gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestMay 30, 2025
…onGH-134769)In121ed71, mt_continue_should_breakwas changed to be guarded by `Py_DEBUG`, but it's used in `compress_mt_continue_lock_held`with just `assert`, so it needs to be available when `NDEBUG` is undefinedtoo.`Py_DEBUG` implies `NDEBUG` is undefined, so we can check just that.(cherry picked from commit2f2bee2)Co-authored-by: Sam James <sam@gentoo.org>Fixes:121ed71
@bedevere-app
Copy link

GH-134916 is a backport of this pull request to the3.14 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.14bugs and security fixes labelMay 30, 2025
@thesamesamthesamesam deleted the py-assertions-vs-zstd branchMay 30, 2025 05:02
gpshead pushed a commit that referenced this pull requestMay 30, 2025
…134769) (#134916)gh-134768: Fix definition of `mt_continue_should_break()` (GH-134769)In121ed71, mt_continue_should_breakwas changed to be guarded by `Py_DEBUG`, but it's used in `compress_mt_continue_lock_held`with just `assert`, so it needs to be available when `NDEBUG` is undefinedtoo.`Py_DEBUG` implies `NDEBUG` is undefined, so we can check just that.(cherry picked from commit2f2bee2)Fixes:121ed71Co-authored-by: Sam James <sam@gentoo.org>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@gpsheadgpsheadgpshead approved these changes

@emmatypingemmatypingemmatyping approved these changes

@erlend-aaslanderlend-aaslandAwaiting requested review from erlend-aasland

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@thesamesam@emmatyping@mgorny@gpshead
[8]ページ先頭
©2009-2025 Movatter.jp