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-39947: revert incorrect change to a comment by iritkatriel · Pull Request #26788 · python/cpython

Conversation

@iritkatriel

@iritkatriel iritkatriel commented Jun 18, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

These functions are called from _PyTrash_begin/_PyTrash_end.

https://bugs.python.org/issue39947

Sorry, something went wrong.

Uh oh!

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

All reactions
@iritkatriel
bpo-39947: revert incorrect change to a comment
c0573bc
@iritkatriel
iritkatriel requested a review from vstinner June 18, 2021 19:21
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jun 18, 2021
@bedevere-bot bedevere-bot added the awaiting core review label Jun 18, 2021
@iritkatriel iritkatriel added the skip news label Jun 18, 2021
@vstinner

vstinner commented Jun 19, 2021

Copy link
Copy Markdown
Member

I'm not sure that this change is correct.

These functions are called from _PyTrash_begin/_PyTrash_end.

Right. But _PyTrash_begin/_PyTrash_end became regular functions, so _PyTrash_thread_deposit_object() and _PyTrash_thread_destroy_chain() don't need to be exported.

They are only exported for the stable ABI.

Maybe the export commands should be moved into object.c? These functions don't need technically to be part of the API.

PyAPI_FUNC(void) _PyTrash_thread_deposit_object(PyObject*);
PyAPI_FUNC(void) _PyTrash_thread_destroy_chain(void);

cc @encukou

All reactions

Sorry, something went wrong.

Uh oh!

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

@iritkatriel

iritkatriel commented Jun 19, 2021

Copy link
Copy Markdown
Member Author

I see. I read the comment to mean "this is no longer used", which I found confusing.

While we're here - do we need to keep the bodies of functions that we keep only for the ABI? Or could we replace the bodies of _PyTrash_deposit_object and _PyTrash_destroy_chain (which are really unused) with a comment saying "this is no longer used, left here for the stable ABI..." ?

All reactions

Sorry, something went wrong.

Uh oh!

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

@encukou

encukou commented Jun 21, 2021

Copy link
Copy Markdown
Member

I'm not that familiar with PyThrash, but the general idea is that macros are compiled into extensions, so extensions will do whatever the macros did in older versions of Python.
If it's now safe to do nothing when those macros call _PyTrash_deposit_object, then the function body can be removed.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Jun 21, 2021

Copy link
Copy Markdown
Member

Until Python 3.8, Py_TRASHCAN_BEGIN_CONDITION emitted a call to the _PyTrash_thread_deposit_object() function. So any C extension built with the stable ABI would call _PyTrash_thread_deposit_object() function at the ABI level. If you modify _PyTrash_thread_deposit_object() to make it a no-op function, it will break these C extensions.

But. The Py_TRASHCAN_BEGIN_CONDITION API was not usable with the limited C API (on Python 3.8 and older) since it accessed to PyThreadState members like _tstate->trash_delete_nesting, whereas PyThreadState structure is opaque in the limited C API. So in Python 3.9, I moved the trashcan C API to the limited C API: https://docs.python.org/dev/whatsnew/3.9.html#id3

At the end, providing any backward compatibility with the stable ABI for Python 3.8 and older sounds useless and maybe we can simply remove the following 4 functions:

/* This is the old private API, invoked by the macros before 3.2.4.
   Kept for binary compatibility of extensions using the stable ABI. */
PyAPI_FUNC(void) _PyTrash_deposit_object(PyObject*);
PyAPI_FUNC(void) _PyTrash_destroy_chain(void);

/* This is the old private API, invoked by the macros before 3.9.
   Kept for binary compatibility of extensions using the stable ABI. */
PyAPI_FUNC(void) _PyTrash_thread_deposit_object(PyObject*);
PyAPI_FUNC(void) _PyTrash_thread_destroy_chain(void);

For the trashcan changes, see:

  • https://bugs.python.org/issue38644
  • https://bugs.python.org/issue39542

When I did these changes, I was afraid of breaking the C API, so I tried to reduce the risk by keeping _PyTrash_thread_deposit_object() and _PyTrash_thread_destroy_chain() "just in case". I was not brave enough to remove them.

All reactions

Sorry, something went wrong.

Uh oh!

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

@encukou

encukou commented Jun 22, 2021

Copy link
Copy Markdown
Member

That sounds correct. Let's remove them from 3.11?

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Jun 23, 2021

Copy link
Copy Markdown
Member

@encukou:

That sounds correct. Let's remove them from 3.11?

Ok, I created PR #26869 to remove the 4 functions.

All reactions

Sorry, something went wrong.

Uh oh!

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

@iritkatriel iritkatriel closed this Jun 23, 2021
@iritkatriel
iritkatriel deleted the bpo-39947 branch June 23, 2021 09:52
@vstinner

vstinner commented Jun 23, 2021

Copy link
Copy Markdown
Member

Ok, I created PR #26869 to remove the 4 functions.

I merged my PR and so removed these 4 functions. Thanks for the reminder @iritkatriel ;-)

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner mentioned this pull request Sep 15, 2022
[C API] Prepare the C API to make PyThreadState opaque: add getter functions #84128
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner Awaiting requested review from vstinner

Uh oh!

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

Assignees

No one assigned

Labels

awaiting core review skip news

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.

5 participants

@iritkatriel @vstinner @encukou @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.