GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-40170: Add PyType_SetBase() function by vstinner · Pull Request #23153 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Nov 4, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@vstinner
vstinner requested review from a team, abalkin, markshannon, pganssle and rhettinger as code owners November 4, 2020 19:42
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Nov 4, 2020
@bedevere-bot bedevere-bot added the awaiting core review label Nov 4, 2020
@vstinner

vstinner commented Nov 4, 2020

Copy link
Copy Markdown
Member Author

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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@shihai1991

shihai1991 commented Nov 5, 2020

Copy link
Copy Markdown
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)?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

shihai1991
shihai1991 reviewed Nov 5, 2020
View reviewed changes
Comment thread Objects/typeobject.c Outdated

@shihai1991 shihai1991 Nov 5, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@vstinner vstinner Nov 5, 2020

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

shihai1991 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@vstinner

vstinner commented Nov 5, 2020

Copy link
Copy Markdown
Member Author

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.

shihai1991 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Pixmew

Pixmew commented Nov 7, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@shihai1991

shihai1991 commented Nov 7, 2020

Copy link
Copy Markdown
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)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Jongy
Jongy reviewed Nov 8, 2020
View reviewed changes
Comment thread Objects/typeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Jongy
Jongy reviewed Nov 8, 2020
View reviewed changes
Comment thread Doc/c-api/type.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@vstinner
bpo-40170: Add PyType_SetBase() function
82bd889
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

vstinner commented Nov 9, 2020

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Nov 9, 2020 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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.

Jongy reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

vstinner commented Nov 9, 2020

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Reviewers

@shihai1991 shihai1991 shihai1991 left review comments
@abalkin abalkin Awaiting requested review from abalkin
@markshannon markshannon Awaiting requested review from markshannon
@pganssle pganssle Awaiting requested review from pganssle
@rhettinger rhettinger Awaiting requested review from rhettinger
+1 more reviewer
@Jongy Jongy Jongy left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@vstinner @shihai1991 @Pixmew @Jongy @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.