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 — gh-103743: Add PyUnstable_Object_GC_NewWithExtraData by jbradaric · Pull Request #103744 · python/cpython

Conversation

@jbradaric

@jbradaric jbradaric commented Apr 24, 2023 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor
  • Issue: Allocating extra data after an object instance #103743

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot

bedevere-bot commented Apr 24, 2023

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot mentioned this pull request Apr 24, 2023
Allocating extra data after an object instance #103743
Closed
@bedevere-bot bedevere-bot added the awaiting review label Apr 24, 2023
@ghost

ghost commented Apr 24, 2023 •
edited by ghost
Loading

Uh oh!

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

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@arhadthedev arhadthedev added the topic-C-API label Apr 24, 2023
@jbradaric
jbradaric force-pushed the jbradaric/obj-new-with-extra-data branch from d045f45 to d300004 Compare April 24, 2023 08:32
Eclips4
Eclips4 reviewed Apr 24, 2023
View reviewed changes
Comment thread Doc/c-api/gcsupport.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@jbradaric
jbradaric force-pushed the jbradaric/obj-new-with-extra-data branch 3 times, most recently from aa3716f to cc46476 Compare April 24, 2023 10:01
@jbradaric
pythongh-103743: Add PyUnstable_Object_GC_NewWithExtraData (python#10…
d644206 
…3743)
@jbradaric
jbradaric force-pushed the jbradaric/obj-new-with-extra-data branch from cc46476 to d644206 Compare April 24, 2023 10:53
@jbradaric
jbradaric marked this pull request as ready for review April 24, 2023 11:20
@jbradaric
jbradaric requested a review from pablogsal as a code owner April 24, 2023 11:20
@jbradaric jbradaric changed the title [WIP] gh-103743: Add PyUnstable_Object_GC_NewWithExtraData gh-103743: Add PyUnstable_Object_GC_NewWithExtraData Apr 24, 2023
ambv and others added 4 commits April 25, 2023 02:34
@ambv
Merge branch 'main' into jbradaric/obj-new-with-extra-data
14975c0
@encukou
Don't suggest PEP 697 will be a solution
52135dc
@encukou
Move declaration to Include/cpython
e2c4b37 
That way it doesn't need the Py_LIMITED_API guard
@encukou
Style nitpicks
ea9e9ac
@encukou
encukou self-requested a review April 27, 2023 16:08
encukou added 3 commits April 28, 2023 14:57
@encukou
Clarify the docs a bit
f9817ca
@encukou
Convert the test class to a heap type
f0098d6
@encukou
Initialize the memory
cfa59d7 
This is a further workaround: this time for the fact that
we can't rely on tp_alloc, which zeroes the memory (including
memory for subclasses, like slots in the test) for normal classes.

For the test type, this means the instances aren't initialized twice
(once in PyUnstable_Object_GC_NewWithExtraData, once again with
PyObject_Init after zeroing). Initializing twice throws off the global
refcount total in debug builds.
@encukou

encukou commented Apr 28, 2023

Copy link
Copy Markdown
Member

Hi,
Time for 3.12 is short, so rather than write review comments I pushed suggestions/fixes to the PR directly. @jbradaric, could you go through the commits (and commit descriptions)? Do they make sense to you?

One more thing to do will be moving GC- or allocation-related tests from _testcapimodule.c to their own file in Modules/_testcapi/, but that can wait for another PR.
erlend-aasland 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.
@jbradaric

jbradaric commented Apr 28, 2023

Copy link
Copy Markdown
Contributor Author

@encukou, commits look good, thanks for the review and fixes. Sorry for the amount of required changes :)
All reactions

Sorry, something went wrong.

Uh oh!

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

jbradaric commented Apr 28, 2023

Copy link
Copy Markdown
Contributor Author

One more thing to do will be moving GC- or allocation-related tests from _testcapimodule.c to their own file in Modules/_testcapi/, but that can wait for another PR.

Would it be OK if I create a PR to move these tests to a separate file? Or is someone already working on this?
erlend-aasland 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.
erlend-aasland
erlend-aasland reviewed Apr 28, 2023
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

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.

LGTM. Minor nits regarding docs.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/c-api/gcsupport.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/c-api/gcsupport.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Misc/NEWS.d/next/C API/2023-04-24-10-31-59.gh-issue-103743.2xYA1K.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@jbradaric @erlend-aasland
Doc style fixes
525705a 
Fix indentation and reword some parts to sound more natural.

Co-authored-by: Erlend E. Aasland <[email protected]>
erlend-aasland
erlend-aasland approved these changes Apr 29, 2023
View reviewed changes
@bedevere-bot bedevere-bot removed the awaiting review label Apr 29, 2023
@bedevere-bot bedevere-bot added the awaiting merge label Apr 29, 2023
@encukou
encukou merged commit 87223f3 into python:main May 2, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label May 2, 2023
@encukou

encukou commented May 2, 2023

Copy link
Copy Markdown
Member

Thank you for your first contribution to CPython! (It is first, right?)

Sorry for the amount of required changes

No problem!

Would it be OK if I create a PR to move these tests to a separate file? Or is someone already working on this?

Go ahead! The issue is #93649 and it has a bunch of example PRs.
I don't think anyone's working on this.
All reactions

Sorry, something went wrong.

Uh oh!

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

jbradaric commented May 2, 2023

Copy link
Copy Markdown
Contributor Author

Thank you for your first contribution to CPython! (It is first, right?)

Yes, it's the first one. Thanks for all the help :)
erlend-aasland reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jbradaric
jbradaric deleted the jbradaric/obj-new-with-extra-data branch May 2, 2023 12:01
carljm added a commit to carljm/cpython that referenced this pull request May 2, 2023
@carljm
Merge branch 'main' into inlinecomp2
0727d6f 
* main:
  pythongh-103822: [Calendar] change return value to enum for day and month APIs (pythonGH-103827)
  pythongh-65022: Fix description of tuple return value in copyreg (python#103892)
  pythonGH-103525: Improve exception message from `pathlib.PurePath()` (pythonGH-103526)
  pythongh-84436: Add integration C API tests for immortal objects (pythongh-103962)
  pythongh-103743: Add PyUnstable_Object_GC_NewWithExtraData (pythonGH-103744)
  pythongh-102997: Update Windows installer to SQLite 3.41.2. (python#102999)
  pythonGH-103484: Fix redirected permanently URLs (python#104001)
  Improve assert_type phrasing (python#104081)
  pythongh-102997: Update macOS installer to SQLite 3.41.2. (pythonGH-102998)
  pythonGH-103472: close response in HTTPConnection._tunnel (python#103473)
  pythongh-88496: IDLE - fix another test on macOS (python#104075)
  pythongh-94673: Hide Objects in PyTypeObject Behind Accessors (pythongh-104074)
  pythongh-94673: Properly Initialize and Finalize Static Builtin Types for Each Interpreter (pythongh-104072)
  pythongh-104016: Skip test for deeply neste f-strings on wasi (python#104071)
carljm added a commit to carljm/cpython that referenced this pull request May 2, 2023
@carljm
Merge branch 'main' into cached_property_docs
b9c39e3 
* main: (760 commits)
  pythonGH-104102: Optimize `pathlib.Path.glob()` handling of `../` pattern segments (pythonGH-104103)
  pythonGH-104104: Optimize `pathlib.Path.glob()` by avoiding repeated calls to `os.path.normcase()` (pythonGH-104105)
  pythongh-103822: [Calendar] change return value to enum for day and month APIs (pythonGH-103827)
  pythongh-65022: Fix description of tuple return value in copyreg (python#103892)
  pythonGH-103525: Improve exception message from `pathlib.PurePath()` (pythonGH-103526)
  pythongh-84436: Add integration C API tests for immortal objects (pythongh-103962)
  pythongh-103743: Add PyUnstable_Object_GC_NewWithExtraData (pythonGH-103744)
  pythongh-102997: Update Windows installer to SQLite 3.41.2. (python#102999)
  pythonGH-103484: Fix redirected permanently URLs (python#104001)
  Improve assert_type phrasing (python#104081)
  pythongh-102997: Update macOS installer to SQLite 3.41.2. (pythonGH-102998)
  pythonGH-103472: close response in HTTPConnection._tunnel (python#103473)
  pythongh-88496: IDLE - fix another test on macOS (python#104075)
  pythongh-94673: Hide Objects in PyTypeObject Behind Accessors (pythongh-104074)
  pythongh-94673: Properly Initialize and Finalize Static Builtin Types for Each Interpreter (pythongh-104072)
  pythongh-104016: Skip test for deeply neste f-strings on wasi (python#104071)
  pythongh-104057: Fix direct invocation of test_super (python#104064)
  pythongh-87092: Expose assembler to unit tests (python#103988)
  pythongh-97696: asyncio eager tasks factory (python#102853)
  pythongh-84436: Immortalize in _PyStructSequence_InitBuiltinWithFlags() (pythongh-104054)
  ...
@brandtbucher

brandtbucher commented May 5, 2023

Copy link
Copy Markdown
Member

FYI, it looks like this USan buildbot started failing on this commit: https://buildbot.python.org/all/#/builders/719

Not sure if it's a false positive or not, but is probably worth looking into (especially since the 3.12 release is coming up).
All reactions

Sorry, something went wrong.

Uh oh!

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

brandtbucher commented May 5, 2023

Copy link
Copy Markdown
Member

Fixed in #104191.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jbradaric jbradaric mentioned this pull request May 11, 2023
gh-93649: Split gc- and allocation tests from _testcapimodule.c #104403
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Eclips4 Eclips4 Eclips4 left review comments
@erlend-aasland erlend-aasland erlend-aasland approved these changes
@pablogsal pablogsal Awaiting requested review from pablogsal pablogsal is a code owner
@encukou encukou Awaiting requested review from encukou

Uh oh!

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

Assignees

No one assigned

Labels

topic-C-API

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.

8 participants

@jbradaric @bedevere-bot @encukou @brandtbucher @erlend-aasland @Eclips4 @arhadthedev @ambv
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.