Skip to content

BUG: fix incorrect reference setting in reduction loop tests - #33175

Closed
ngoldbaum wants to merge 1 commit into
numpy:mainfrom
ngoldbaum:fix-loop-test-refcounting
Closed

ngoldbaum wants to merge 1 commit into
numpy:mainfrom
ngoldbaum:fix-loop-test-refcounting

Conversation

@ngoldbaum

Copy link
Copy Markdown
Member

PR summary

My clanker found this while reviewing #32213. That PR adds tests that trigger a refcounting bug because of this.

This extension is only used in tests and the real minimummaximum implementation does this correctly.

AI Disclosure

An AI model noticed the issue.

@ikrommyd ikrommyd left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I remember this came from #32256
Doesn't this break the limited api requirement?
AFAIK Py_XSETREF is not in the limited api (yet at least)? Unless I'm wrong.
Do we need #32871 here?

Edit: Ah I see Pratham is touching exactly this code in #32871

@ngoldbaum

Copy link
Copy Markdown
Member Author

Ah I see Pratham is touching exactly this code in #32871

Merging that is blocked on python/pythoncapi-compat#187. IMO we should fix this right now because the current code is wrong. Then when the compat headers grow a definition this will just work.

@ngoldbaum

Copy link
Copy Markdown
Member Author

Ugh except the tests are failing...

@ngoldbaum

Copy link
Copy Markdown
Member Author

Closing in favor of #32871.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants