Skip to content

bpo-40170: Add PyType_SetBase() function #23153

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

Closed
wants to merge 1 commit into from
Closed

bpo-40170: Add PyType_SetBase() function #23153

wants to merge 1 commit into from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Nov 4, 2020

Add PyType_SetBase() and PyType_SetBaseStatic() functions to set the
base of a type.

PyType_Ready() no longer increment the reference count of the object
type on static types which have no base type and use a borrowed
reference to their base type.

https://bugs.python.org/issue40170

@vstinner
Copy link
Member Author

vstinner commented Nov 4, 2020

After Py_SET_TYPE(), here is a new function to abstract access to the PyTypeObject structure: PyType_SetBase() to set the tp_base member.

PyType_SetBase() is safer than a direct access to tp_base since it handles reference count for the caller.

PyType_SetBaseStatic() is also provided for compatibility with extension modules using static types: set tp_base member, but don't touch reference count.

The PR also fix a reference leak in PyType_Ready():

PyType_Ready() no longer increment the reference count of the object
type on static types which have no base type and use a borrowed
reference to their base type.

cc @nascheme @corona10 @shihai1991 @encukou @pablogsal

@shihai1991
Copy link
Member

After Py_SET_TYPE(), here is a new function to abstract access to the PyTypeObject structure: PyType_SetBase() to set the tp_base member.

PyType_SetBase() is safer than a direct access to tp_base since it handles reference count for the caller.

PyType_SetBaseStatic() is also provided for compatibility with extension modules using static types: set tp_base member, but don't touch reference count.

The PR also fix a reference leak in PyType_Ready():

PyType_Ready() no longer increment the reference count of the object
type on static types which have no base type and use a borrowed
reference to their base type.

cc @nascheme @corona10 @shihai1991 @encukou @pablogsal

Nice improvement.
Can we make PyType_SetBase() more universal? something like: PyType_SetSlot(type, Py_tp_base, base)?

type->tp_bases = bases;
bases = NULL;
Py_INCREF(base);
type->tp_base = base;
PyType_SetBase(type, base);
Copy link
Member

Choose a reason for hiding this comment

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

If Line 2995 can be replaced, why Line 842 can not do the same operation?
https://github.com/python/cpython/blob/master/Objects/typeobject.c#L842

Copy link
Member Author

Choose a reason for hiding this comment

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

type_set_bases() is very complex, I prefer to leave it unchanged to reduce the risk of any regression. IMHO the current type_set_bases() code is simpler to follow without an indirection like PyType_SetBase() function call.

@vstinner
Copy link
Member Author

vstinner commented Nov 5, 2020

Can we make PyType_SetBase() more universal? something like: PyType_SetSlot(type, Py_tp_base, base)?

It would be possible, but I prefer one function per PyTypeObject member.

For example, PyType_SetBase() cannot fail. A function which would accept a parameter to choose the slot would introduce error handling, to handle invalid/unknown slot.

@Pixmew
Copy link
Contributor

Pixmew commented Nov 7, 2020

hello sir i accidently removed my reviewer and now it is saying awating core review but there are no one under reviewer section. can you plese help me

it is not saying like yours
Review has been requested on this pull request. It is not required to merge
my PR is #23166 can you please check it
thankyou

@shihai1991
Copy link
Member

hello sir i accidently removed my reviewer and now it is saying awating core review but there are no one under reviewer section. can you plese help me

it is not saying like yours
Review has been requested on this pull request. It is not required to merge
my PR is #23166 can you please check it
thankyou

Hello, Yash.
MAYBE you can find the maintainer or experts of purges.py in https://devguide.python.org/experts/.
Or ping steve Dower in your PR directly(I saw steve is the author of purges.py in https://github.com/python/cpython/blob/master/Tools/msi/purge.py#L7)

Add PyType_SetBase() and PyType_SetBaseStatic() functions to set the
base of a type.

PyType_Ready() no longer increment the reference count of the object
type on static types which have no base type and use a borrowed
reference to their base type.
@vstinner
Copy link
Member Author

vstinner commented Nov 9, 2020

I rebased my PR and fixed the typo spotted by @Jongy.

@vstinner
Copy link
Member Author

vstinner commented Nov 9, 2020

Wait, this PR doesn't make any sense.

The purpose of bpo-40170 is to move the C API towards opaque PyTypeObject structure. This PR adds PyType_SetBase() for heap types and PyType_SetBaseStatic() for static types.

PyType_SetBase() is only used in typeobject.c because it doesn't make sense to update tp_base after a type is created. So PyType_SetBase() is useless.

PyType_SetBaseStatic() doesn't solve bpo-40170: defining a static types requires that the PyTypeObject structure is not opaque. It is not possible to define a type statically if PyTypeObject is opaque. So PyType_SetBaseStatic() is useless.

This PR is useless, I close it. Moreover, PyType_FromSpecWithBases() can be used to create a heap type with a base type.

@vstinner vstinner closed this Nov 9, 2020
@vstinner vstinner deleted the pytype_setbase branch November 9, 2020 16:04
@vstinner
Copy link
Member Author

vstinner commented Nov 9, 2020

I saw a lot of code setting tp_base on types, but I didn't notice that all these types were statically defined.

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.

6 participants