Skip to content

Conversation

@JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented Oct 17, 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, thanks!

@ZeroIntensity
Copy link
Member

Is there any backport needed here? I'm not sure if the original PR made it in 3.13

@JelleZijlstra
Copy link
Member Author

No, this is a PEP 649 change and is only on main.

Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

Thanks Jelle!

A

@JelleZijlstra JelleZijlstra merged commit f203d1c into python:main Oct 17, 2024
40 checks passed
@JelleZijlstra JelleZijlstra deleted the classmanno branch October 17, 2024 16:45
@zware
Copy link
Member

zware commented Oct 17, 2024

Heads-up: this appears to have caused some refleak buildbot failures, see for example https://buildbot.python.org/#/builders/1613/builds/116

zware added a commit to zware/cpython that referenced this pull request Oct 17, 2024
zware added a commit that referenced this pull request Oct 17, 2024
@ZeroIntensity
Copy link
Member

Actually, on a second look, this part is technically unsafe:

else {
    Py_DECREF(dict);
    return PyDict_SetItem(dict, name, value);
}

If the dictionary's reference count is 1, then this will result in a use-after-free. Though, I guess that's never possible because the dictionary will be stored on the instance as well. In hindsight, there should have either been an assertion (or just not do it in the first place), but oh well.

ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants