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

Jump to bottom

gh-93442: Make C++ version of _Py_CAST work with 0/NULL. #93500

Merged
merged 2 commits into from Jun 5, 2022
Merged

gh-93442: Make C++ version of _Py_CAST work with 0/NULL. #93500

merged 2 commits into from Jun 5, 2022

Conversation

nascheme
Copy link
Member

@nascheme nascheme commented Jun 4, 2022

Add C++ overloads for _Py_CAST_impl() to handle 0/NULL. This will allow
C++ extensions that pass 0 or NULL to macros using _Py_CAST() to
continue to compile. Without this, you get an error like:

invalid ‘static_cast’ from type ‘int’ to type ‘_object*’

The modern way to use a NULL value in C++ is to use nullptr. However,
we want to not break extensions that do things the old way.

Co-authored-by: serge-sans-paille

@nascheme
Make C++ version of _Py_CAST work with 0/NULL.
8c0e689 
Add C++ overloads for _Py_CAST_impl() to handle 0/NULL.  This will allow
C++ extensions that pass 0 or NULL to macros using _Py_CAST() to
continue to compile.  Without this, you get an error like:

    invalid ‘static_cast’ from type ‘int’ to type ‘_object*’

The modern way to use a NULL value in C++ is to use nullptr.  However,
we want to not break extensions that do things the old way.

Co-authored-by: serge-sans-paille
@nascheme nascheme linked an issue Jun 4, 2022 that may be closed by this pull request
PyTuple_SET_ITEM fails to compile in C++ source #93442
Closed
@corona10 corona10 changed the title Make C++ version of _Py_CAST work with 0/NULL. gh-93442: Make C++ version of _Py_CAST work with 0/NULL. Jun 5, 2022
corona10
corona10 approved these changes Jun 5, 2022
View changes
Lib/test/_testcppext.cpp
// gh-93442: Pass 0 as NULL for PyObject*
Py_XINCREF(0);
Py_XDECREF(0);

Copy link
Member

@corona10 corona10 Jun 5, 2022

Choose a reason for hiding this comment

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

nit

Suggested change
Py_XINCREF(nullptr);
Py_XDECREF(nullptr);
Py_XINCREF(NULL);
Py_XDECREF(NULL);

@nascheme nascheme merged commit 8bcc3fa into python:main Jun 5, 2022
13 checks passed
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jun 5, 2022

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

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jun 5, 2022

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

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jun 5, 2022
@nascheme @miss-islington
pythongh-93442: Make C++ version of _Py_CAST work with 0/NULL. (pytho…
3c64fa3 
…nGH-93500)

Add C++ overloads for _Py_CAST_impl() to handle 0/NULL.  This will allow
C++ extensions that pass 0 or NULL to macros using _Py_CAST() to
continue to compile.  Without this, you get an error like:

    invalid ‘static_cast’ from type ‘int’ to type ‘_object*’

The modern way to use a NULL value in C++ is to use nullptr.  However,
we want to not break extensions that do things the old way.

Co-authored-by: serge-sans-paille
(cherry picked from commit 8bcc3fa)

Co-authored-by: Neil Schemenauer <nas-github@arctrix.com>
corona10 pushed a commit that referenced this issue Jun 5, 2022
@miss-islington @nascheme
gh-93442: Make C++ version of _Py_CAST work with 0/NULL. (GH-93500) (g…
d97e2c5 
…h-93507)

Add C++ overloads for _Py_CAST_impl() to handle 0/NULL.  This will allow
C++ extensions that pass 0 or NULL to macros using _Py_CAST() to
continue to compile.  Without this, you get an error like:

    invalid ‘static_cast’ from type ‘int’ to type ‘_object*’

The modern way to use a NULL value in C++ is to use nullptr.  However,
we want to not break extensions that do things the old way.

Co-authored-by: serge-sans-paille
(cherry picked from commit 8bcc3fa)

Co-authored-by: Neil Schemenauer <nas-github@arctrix.com>

Co-authored-by: Neil Schemenauer <nas-github@arctrix.com>
@vstinner
Copy link
Member

@vstinner vstinner commented Jun 6, 2022

Since there is a _Py_CAST_impl() implementation for nullptr, it would be nice to add a test on nullptr.

What is the purpose of _Py_CAST_impl(long int ptr)? Is it to pass a pointer as a number? Or is it to pass 0L or (long)0? Who uses such code?

@nascheme
Copy link
Member Author

@nascheme nascheme commented Jun 6, 2022

I added a test for nullptr. The reason for the int overloads is because the C++ template type is created from the expression, not the type it is being coerced to. So, _Py_CAST(0) or _Py_CAST(NULL) expands the template with long int or similar as the type. I think the long int is due to what type C++ uses for a literal 0 value.

I'm a bit scared about how complicated _Py_CAST_impl() has become. AFAICT, it only exists to avoid a gcc warning. It's nice to avoid the warning but my concern is if there are more issues and the long term maintenance of that "magic" code. Since it seems to work, I guess we should keep it.

@nascheme nascheme deleted the pycast_cpp_null branch Jun 6, 2022
@vstinner
Copy link
Member

@vstinner vstinner commented Jun 6, 2022

I'm a bit scared about how complicated _Py_CAST_impl() has become. AFAICT, it only exists to avoid a gcc warning.

It's more about generating "correct" code for a very tricky operation: convert any operation to PyObject*. With PEP 670, the limited C API version 3.11 simply removes the cast: you must pass PyOject* and only PyObject*. The complexity of the C++ implementation is more the cost of the past, keeping backward compatibility without emiting new C++ compiler warnings.

But you're right that my initial concern was fixing new C++ compiler warnings: to fix a regression. See #91320 (comment) for the rationale and why it's a regression.

@vstinner
Copy link
Member

@vstinner vstinner commented Jun 6, 2022

I added a test for nullptr

Ah right, commit 713eb18: thanks!

@vstinner
Copy link
Member

@vstinner vstinner commented Jun 6, 2022

test_cppext is new in Python 3.11! Previously, there was simply zero test on the C++ compatibility of the Python C API ;-)

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

@corona10 corona10

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PyTuple_SET_ITEM fails to compile in C++ source
5 participants
@nascheme @miss-islington @bedevere-bot @vstinner @corona10