Skip to content

MAINT: make Py_SET_SIZE and Py_SET_TYPE macros a bit safer #16501

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

Merged
merged 2 commits into from
Jun 6, 2020

Conversation

tacaswell
Copy link
Contributor

This is the spelling of the compatibility macro that CPython is
recommending.

See python/cpython#20610

This is the spelling of the compatibility macro that CPython is
recommending.
@eric-wieser
Copy link
Member

This looks less safe to me, because now it can't be used in an expression like (Py_SET_TYPE(x, y), z), which is legal in 3.10.

@tacaswell
Copy link
Contributor Author

I am going to back out of this discussion as I am out of my depth on c/c++ details and let you and Victor sort this out :)

@tacaswell tacaswell closed this Jun 4, 2020
@eric-wieser
Copy link
Member

Let's leave this open to track getting in line with python - we should match what ultimately gets merged in that PR.

@eric-wieser eric-wieser reopened this Jun 4, 2020
@seberg
Copy link
Member

seberg commented Jun 4, 2020

Would it be OK to make it a milestones issue rather?

Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

From the other PR

@eric-wieser
Copy link
Member

@seberg, this is valuable as an open PR because it means it's super easy to edit in-browser :)

@vstinner
Copy link
Contributor

vstinner commented Jun 4, 2020

This looks less safe to me, because now it can't be used in an expression like (Py_SET_TYPE(x, y), z), which is legal in 3.10.

Do you mean that python/cpython#20610 should suggest to use #define Py_SET_SIZE(obj, size) (Py_SIZE(obj) = (size)) ?

@eric-wieser
Copy link
Member

eric-wieser commented Jun 4, 2020

The variant I suggested over there using (void)0 is better than both the original here and the original there. Our version was unsafe because it allowed Py_SET_TYPE(x, y) = z, which obviously won't work in 3.9. Your version was overly strict. The version that is now in both patches should be exactly the right level of strict.

@seberg
Copy link
Member

seberg commented Jun 4, 2020

Did not expect as fast turn-around... I am half expecting that in the end, there may be some python header we can vendor in here instead. But for now all good as soon as tests pass...

I don't mind these small changes there are easy enough, just if there is a mass of tiny macro fixes we backport one-by-one it will get annoying.

@vstinner
Copy link
Contributor

vstinner commented Jun 4, 2020

Did not expect as fast turn-around... I am half expecting that in the end, there may be some python header we can vendor in here instead.

I started a thread on the capi-sig list to propose exactly that:
https://mail.python.org/archives/list/capi-sig@python.org/thread/HUCHXBYCHL6ZXPZYOEEATXTKMQLAYVEA/

@eric-wieser eric-wieser merged commit f167107 into numpy:master Jun 6, 2020
@tacaswell tacaswell deleted the mnt_310_macro_safety branch July 20, 2020 14:23
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