Skip to content

gh-94731: Revert to C-style casts for _Py_CAST #94782

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 8 commits into from
Jul 14, 2022

Conversation

encukou
Copy link
Member

@encukou encukou commented Jul 12, 2022

@encukou encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 13, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @encukou for commit 498e8bb 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 13, 2022
@encukou encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 13, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @encukou for commit b13e464 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 13, 2022
Comment on lines 170 to 175
PyObject *ipotype = PyType_FromSpec(&VirtualPyObject_Spec);
// no good way to signal failure from a C++ constructor, so use assert
// for error handling
assert(ipotype);
assert(PyObject_Init(this, (PyTypeObject *)ipotype));
Py_DECREF(ipotype);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
PyObject *ipotype = PyType_FromSpec(&VirtualPyObject_Spec);
// no good way to signal failure from a C++ constructor, so use assert
// for error handling
assert(ipotype);
assert(PyObject_Init(this, (PyTypeObject *)ipotype));
Py_DECREF(ipotype);
PyObject *vpotype = PyType_FromSpec(&VirtualPyObject_Spec);
// no good way to signal failure from a C++ constructor, so use assert
// for error handling
assert(vpotype);
assert(PyObject_Init(this, (PyTypeObject *)vpotype));
Py_DECREF(vpotype);

Just suggesting changing the name slightly here to match the name you used from the class. I think the i from ipotype was a hold-over from the name I used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! It's a local variable now so I'll just name it type :)

@encukou encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 13, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @encukou for commit 021c03d 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 13, 2022
@encukou encukou merged commit 6cbb57f into python:main Jul 14, 2022
@miss-islington
Copy link
Contributor

Thanks @encukou for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@encukou encukou deleted the c-style-cast branch July 14, 2022 09:57
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 14, 2022
Co-authored-by: da-woods <dw-git@d-woods.co.uk>
(cherry picked from commit 6cbb57f)

Co-authored-by: Petr Viktorin <encukou@gmail.com>
@bedevere-bot
Copy link

GH-94849 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Jul 14, 2022
encukou pushed a commit that referenced this pull request Jul 15, 2022
Co-authored-by: da-woods <dw-git@d-woods.co.uk>
(cherry picked from commit 6cbb57f)
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants