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-32388: Remove cross-version binary compatibility requirement in tp_flags by pitrou · Pull Request #4944 · python/cpython

Conversation

@pitrou

@pitrou pitrou commented Dec 20, 2017 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Starting with 3.8, it is now allowed to add new fields at the end of the PyTypeObject struct in feature releases without having to allocate a dedicated compatibility flag in tp_flags. This will reduce the risk of running out of bits in the 32-bit tp_flags value.

https://bugs.python.org/issue32388

Sorry, something went wrong.

Uh oh!

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

All reactions
@pitrou
pitrou requested review from 1st1 and asvetlov as code owners December 20, 2017 17:29
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Dec 20, 2017
@bedevere-bot bedevere-bot added the awaiting merge label Dec 20, 2017
@pitrou
pitrou requested a review from serhiy-storchaka December 21, 2017 19:39
scoder
scoder reviewed Jun 20, 2018
View reviewed changes
Comment thread Include/object.h Resolved

Uh oh!

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

JimJJewett
JimJJewett reviewed Jun 20, 2018
View reviewed changes
Comment thread Objects/object.c Resolved

Uh oh!

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

@jdemeyer

jdemeyer commented Jul 31, 2018

Copy link
Copy Markdown
Contributor

You write "Starting with 3.7, binary compatibility of C extensions accross feature releases of Python is not supported anymore" but has such binary compatibility ever been supported? This PR seems to imply that it was, but PEP 384 explicitly says that the layout of PyTypeObject is not part of the stable ABI.

PS: typo accross -> across

All reactions

Sorry, something went wrong.

Uh oh!

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

@pitrou pitrou closed this May 6, 2019
@pitrou
pitrou force-pushed the no_tpflags_compatibility branch from d49cf6f to f7b494c Compare May 6, 2019 16:30
@pitrou pitrou reopened this May 6, 2019
@pitrou
bpo-32388: Remove cross-version binary compatibility requirement in t…
19770ae
…p_flags

It is now allowed to add new fields at the end of the PyTypeObject struct
(and even in the middle, if we are so inclined) in feature releases without
having to allocate a dedicated compatibility flag in tp_flags.

This will reduce the risk of running out of bits in the 32-bit tp_flags value.
@pitrou
pitrou force-pushed the no_tpflags_compatibility branch from 57ea6a3 to 19770ae Compare May 6, 2019 16:33
@pitrou

pitrou commented May 6, 2019

Copy link
Copy Markdown
Member Author

I updated this PR against master and addressed the review comments.

All reactions

Sorry, something went wrong.

Uh oh!

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

@pitrou

pitrou commented May 6, 2019

Copy link
Copy Markdown
Member Author

@zooba Is there a way to retry an Azure job which failed because it couldn't "provision the agent"?

All reactions

Sorry, something went wrong.

Uh oh!

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

tirkarthi
tirkarthi reviewed May 6, 2019
View reviewed changes
Comment thread Doc/whatsnew/3.8.rst Resolved

Uh oh!

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

jdemeyer
jdemeyer reviewed May 6, 2019
View reviewed changes
Comment thread Include/object.h Resolved

Uh oh!

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

@pitrou
Address review comments
9182634
@pitrou pitrou added awaiting core review and removed awaiting merge labels May 6, 2019
@jdemeyer

jdemeyer commented May 15, 2019

Copy link
Copy Markdown
Contributor

It would be good to get this done before the first 3.8 beta.

@scoder: is there any chance that you could finish the "core review"?

All reactions

Sorry, something went wrong.

Uh oh!

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

@jdemeyer

jdemeyer commented May 15, 2019

Copy link
Copy Markdown
Contributor

Starting with 3.8, it is now allowed to add new fields at the end of the PyTypeObject struct (and even in the middle, if we are so inclined)

Not "in the middle" as that would break the typical code of the form

PyTypeObject PyCFunction_Type = {
    PyVarObject_HEAD_INIT(&PyType_Type, 0)
    "builtin_function_or_method",
    sizeof(PyCFunctionObject),
    0,
    (destructor)meth_dealloc,                   /* tp_dealloc */
    0,                                          /* tp_print */
    0,                                          /* tp_getattr */
    0,                                          /* tp_setattr */
    0,                                          /* tp_reserved */
    (reprfunc)meth_repr,                        /* tp_repr */
    0,                                          /* tp_as_number */
    0,                                          /* tp_as_sequence */
    0,                                          /* tp_as_mapping */
    (hashfunc)meth_hash,                        /* tp_hash */
    PyCFunction_Call,                           /* tp_call */
    0,                                          /* tp_str */
    PyObject_GenericGetAttr,                    /* tp_getattro */
    0,                                          /* tp_setattro */
    0,                                          /* tp_as_buffer */
    Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC,/* tp_flags */
    0,                                          /* tp_doc */
    (traverseproc)meth_traverse,                /* tp_traverse */
    0,                                          /* tp_clear */
    meth_richcompare,                           /* tp_richcompare */
    offsetof(PyCFunctionObject, m_weakreflist), /* tp_weaklistoffset */
    0,                                          /* tp_iter */
    0,                                          /* tp_iternext */
    meth_methods,                               /* tp_methods */
    meth_members,                               /* tp_members */
    meth_getsets,                               /* tp_getset */
    0,                                          /* tp_base */
    0,                                          /* tp_dict */
};

With C99, there are better ways to write the above, but we should still support that.

pitrou 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.

@jdemeyer

jdemeyer commented May 15, 2019

Copy link
Copy Markdown
Contributor

One thing that keeps bugging me when I see this diff: Do we really have to change the value of Py_TPFLAGS_HAVE_FINALIZE? I don't see any immediate problem with changing the value, but it would be more prudent to keep it as 1 << 0.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented May 27, 2019

Copy link
Copy Markdown
Member

cc @encukou

All reactions

Sorry, something went wrong.

Uh oh!

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

@encukou

encukou commented May 28, 2019

Copy link
Copy Markdown
Member

Can we keep the bit for backwards compatibility?
We're not running out of bits any time soon, especially since we'll not be adding one for each new feature. (Even though I want to add two of them now.)

Pedantically, something like MY_USUAL_FLAGS & ~Py_TPFLAGS_HAVE_FINALIZE would be correct in 3.7, so it should work in 3.8 as well.

All reactions

Sorry, something went wrong.

Uh oh!

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

@pitrou

pitrou commented May 29, 2019

Copy link
Copy Markdown
Member Author

Pedantically, something like MY_USUAL_FLAGS & ~Py_TPFLAGS_HAVE_FINALIZE would be correct in 3.7, so it should work in 3.8 as well.

What value do you suggest Py_TPFLAGS_HAVE_FINALIZE should have then?

All reactions

Sorry, something went wrong.

Uh oh!

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

@pitrou
Merge branch 'master' into no_tpflags_compatibility
7ce291f
@jdemeyer

jdemeyer commented May 29, 2019

Copy link
Copy Markdown
Contributor

What value do you suggest Py_TPFLAGS_HAVE_FINALIZE should have then?

The same value that it always had.

encukou 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.

@pitrou
Restore previous tp_flag value
8dc3ca2
@pitrou

pitrou commented May 29, 2019

Copy link
Copy Markdown
Member Author

Ok. Unless someone opposes, I'll merge this once CI is green.

encukou 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.

@pitrou
pitrou merged commit ada319b into python:master May 29, 2019
@bedevere-bot bedevere-bot removed the awaiting core review label May 29, 2019
@pitrou
pitrou deleted the no_tpflags_compatibility branch May 29, 2019 20:12
@encukou

encukou commented May 29, 2019

Copy link
Copy Markdown
Member

Thank you!

All reactions

Sorry, something went wrong.

Uh oh!

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

@scoder

scoder commented Jun 1, 2019

Copy link
Copy Markdown
Contributor

Nice, happy to see that this was merged.

All reactions

Sorry, something went wrong.

Uh oh!

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

@jdemeyer jdemeyer mentioned this pull request Jun 6, 2019
bpo-36974: inherit tp_vectorcall_offset unconditionally #13858
Merged
@jdemeyer jdemeyer mentioned this pull request Jul 2, 2019
Typo fix in file Include/object.h #14556
Merged
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@pitrou @DinoV
bpo-32388: Remove cross-version binary compatibility requirement in t…
6275b36
…p_flags (pythonGH-4944)

It is now allowed to add new fields at the end of the PyTypeObject struct without having to allocate a dedicated compatibility flag in tp_flags.

This will reduce the risk of running out of bits in the 32-bit tp_flags value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scoder scoder scoder left review comments
@jdemeyer jdemeyer jdemeyer left review comments
@tirkarthi tirkarthi tirkarthi left review comments
@1st1 1st1 Awaiting requested review from 1st1
@asvetlov asvetlov Awaiting requested review from asvetlov
@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka
+1 more reviewer
@JimJJewett JimJJewett JimJJewett 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

None yet

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.

9 participants

@pitrou @jdemeyer @vstinner @encukou @scoder @tirkarthi @JimJJewett @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.