Skip to content
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

gh-127119: Remove check on small ints in long_dealloc #127120

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

eendebakpt
Copy link
Contributor

@eendebakpt eendebakpt commented Nov 21, 2024

The python small ints are immortal with a fixed reference count. For that reason these objects can never reach reference count zero and are never deallocated. We can remove the special casing in long_dealloc. For safety we replace it with an assert.

This check was found when adding a freelist for compact ints, see #126865

@skirpichev
Copy link
Member

Apparently, assertion is broken in tests.

@eendebakpt
Copy link
Contributor Author

Apparently, assertion is broken in tests.

Yes, my mistake. Should be fixed now

ZeroIntensity
ZeroIntensity previously approved these changes Nov 22, 2024
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

LGTM

@ZeroIntensity ZeroIntensity added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Nov 22, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @ZeroIntensity for commit 4dc97c9 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Nov 22, 2024
@ZeroIntensity ZeroIntensity dismissed their stale review November 22, 2024 15:12

CI is failing, because apparently you can build with assertions but not with debug enabled (that's what I get for prematurely approving, ha).

Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

LGTM (again)!

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