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-98724: Fix the Py_CLEAR() macro in the limited C API by vstinner · Pull Request #100121 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Dec 8, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

If _Py_TYPEOF() is not available, the limited C API now implements the Py_CLEAR() macro as a function call which hides implementation details. The function uses memcpy() of <string.h> which is not included by <Python.h>.

  • Add private _Py_Clear() function.
  • Add _Py_Clear() to Misc/stable_abi.toml.
  • Issue: Macro Py_CLEAR references argument two times. #98724

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner vstinner added the skip news label Dec 8, 2022
@bedevere-bot bedevere-bot mentioned this pull request Dec 8, 2022
Macro Py_CLEAR references argument two times. #98724
Closed
@netlify

netlify Bot commented Dec 8, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

✅ Deploy Preview for python-cpython-preview canceled.

Name Link
🔨 Latest commit 5dac437
🔍 Latest deploy log https://app.netlify.com/sites/python-cpython-preview/deploys/63926d360234d3000869cffc
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting core review label Dec 8, 2022
@vstinner

vstinner commented Dec 8, 2022

Copy link
Copy Markdown
Member Author

Oh, I forgot that Py_CLEAR() is already part of the limited C API and the limited C API no longer includes <string.h> since Python 3.11.

I already proposed a few times to provide Py_SETREF() and Py_CLEAR() as function calls. Well, here is my attempt to introduce a function implementation of the Py_CLEAR() macro :-)

If Py_SETREF() and Py_XSETREF() macros are added to the limited C API (PR #99575), they will need a similar function implementation.

I would really prefer hiding the implementation details and avoiding to include <string.h> in the limited C API <Python.h>.

cc @serhiy-storchaka @encukou
All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented Dec 8, 2022

Copy link
Copy Markdown
Member Author

In this PR, I chose not use the function implementation if the __typeof__() function is available for best performance.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner vstinner mentioned this pull request Dec 8, 2022
gh-99574: Add Py_SETREF() to the limited C API #99575
Closed
@vstinner
vstinner requested a review from a team as a code owner December 8, 2022 23:00
@vstinner
gh-98724: Fix the Py_CLEAR() macro in the limited C API
5dac437 
If _Py_TYPEOF() is not available, the limited C API now implements
the Py_CLEAR() macro as a function call which hides implementation
details. The function uses memcpy() of <string.h> which is not
included by <Python.h>.

* Add private _Py_Clear() function.
* Add _Py_Clear() to Misc/stable_abi.toml.
@serhiy-storchaka

serhiy-storchaka commented Dec 9, 2022

Copy link
Copy Markdown
Member

I suggest to not fix what was not broken. Py_CLEAR and Py_SETREF were macros, and worked pretty well as macros. Problems started when you converted them to functions.
All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented Dec 9, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

I suggest to not fix what was not broken.

I'm not sure what you are talking about, the issue #98724 described a bug that was fixed in Python 3.12.

Py_CLEAR and Py_SETREF were macros, and worked pretty well as macros. Problems started when you converted them to functions.

Py_CLEAR() and Py_SETREF() are currently implemented as macro. What makes you think that they became functions?

This change is specific to the limited API.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka

serhiy-storchaka commented Dec 9, 2022

Copy link
Copy Markdown
Member

I do not think that it is a bug. The documentation can be improved, if it confuses somebody.

If Py_CLEAR is a macro, no need to introduce _Py_Clear. It is a prelimilary pessimization.
All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented Dec 9, 2022

Copy link
Copy Markdown
Member Author

If Py_CLEAR is a macro, no need to introduce _Py_Clear. It is a prelimilary pessimization.

Right now, calling Py_CLEAR() in a C extension using the limited C API fails because memcpy() is not defined. That's the problem I'm trying to solve.

I do not think that it is a bug. The documentation can be improved, if it confuses somebody.

About the issue #98724 : yes, it would be possible to document that Py_CLEAR() and Py_SETREF() have bugs and there are ways to work around them.

I chose to fix the bug instead. I prefer to provide an API which is not error prone. I would like to promote Py_CLEAR() and Py_SETREF() "functions" (macros), they are preventing race conditions and so more code should use them. But I would not be comfortable to have to explain that they have bugs.

Fixing that bugs makes me me comfortable to add these functions to the limited C API.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka

serhiy-storchaka commented Dec 9, 2022

Copy link
Copy Markdown
Member

I do not think there was a bug. By fixing a non-bug you introduced a bug which you are trying to fix now.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner vstinner closed this Dec 13, 2022
@vstinner
vstinner deleted the py_clear_func branch December 13, 2022 15:23
@fuhsnn fuhsnn mentioned this pull request Jul 12, 2024
Define _Py_TYPEOF on MSVC and other compilers supporting __typeof__ #121647
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

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.

3 participants

@vstinner @serhiy-storchaka @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.