Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-117511: Make PyMutex public in the non-limited API#117731

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
colesbury merged 14 commits intopython:mainfromcolesbury:gh-117511-pymutex
Jun 20, 2024

Conversation

colesbury
Copy link
Contributor

@colesburycolesbury commentedApr 10, 2024•
edited
Loading

colesburyand others added7 commitsApril 11, 2024 11:53
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Update code comments to further clarify that the `_bits` field is notpart of the public C API.
@colesburycolesbury marked this pull request as ready for reviewJune 17, 2024 19:38
@colesburycolesbury requested a review froma team as acode ownerJune 17, 2024 19:38
@colesbury
Copy link
ContributorAuthor

@encukou@vstinner - I've updated the docs based oncapi-workgroup/decisions#22 (comment).

I'm not sure of the best way to make the functions also exported as regular functions, particularly with the same name. It's easy to export them with different names and_PyMutex_LockSlow and_PyMutex_UnlockSlow are effectively that.

@vstinner
Copy link
Member

I'm not sure of the best way to make the functions also exported as regular functions, particularly with the same name.

You can look at my PR which does exactly that just for Py_TYPE():https://github.com/python/cpython/pull/120601/files

In short, add an underscore prefix to the static inline function, then use a macro to "rename" the static inline function.

colesbury reacted with thumbs up emoji


#ifndef Py_LIMITED_API
# define Py_CPYTHON_LOCK_H
# include "cpython/lock.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I don't see the value of such header file. Just include cpython/lock.h in Python.h, and check Py_LIMITED_API in cpython/lock.h.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@ericsnowcurrently expressed a preference for this style whenpyatomic.h was added:#109344 (comment)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

IMO, it's helpful to follow a consistent pattern when it comes to the Include/cpython header files. That means in some cases we end up with very minimal header files like this in Include/.

colesbury and encukou reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

If you mention consistency, there are already many header files in Include/cpython/ which have no companion Include/ header file:

$ grep cpython/ Include/Python.h#include "cpython/pydebug.h"#include "cpython/longintrepr.h"#include "cpython/odictobject.h"#include "cpython/funcobject.h"#include "cpython/classobject.h"#include "cpython/code.h"#include "cpython/cellobject.h"#include "cpython/initconfig.h"#include "cpython/genobject.h"#include "cpython/picklebufobject.h"#include "cpython/pytime.h"#include "cpython/context.h"#include "cpython/pyctype.h"#include "cpython/pyfpe.h"#include "cpython/tracemalloc.h"#include "cpython/optimizer.h"

@@ -2372,7 +2372,7 @@ new_reference(PyObject *op)
#else
op->ob_tid = _Py_ThreadId();
op->_padding = 0;
op->ob_mutex = (struct _PyMutex){ 0 };
op->ob_mutex = (PyMutex){ 0 };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

If there is no public PyMutex_STATIC_INIT, can you maybe add a private one in pycore_lock.h?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Let's consider that in a separate PR:

  • It's not directly related to making the API public
  • It would make sense to consider that for multiple types (e.g.,PyEvent,_PyOnceFlag), not justPyMutex

vstinner reacted with thumbs up emoji
Python/lock.c Outdated
void
_PyMutex_LockSlow(PyMutex *m)
PyMutex_Lock(PyMutex *m)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

You might move it to the end to still use the static inline macro in the following lines.

colesbury reacted with thumbs up emoji
@colesburycolesbury added the needs backport to 3.13bugs and security fixes labelJun 18, 2024

#ifndef Py_LIMITED_API
# define Py_CPYTHON_LOCK_H
# include "cpython/lock.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

If you mention consistency, there are already many header files in Include/cpython/ which have no companion Include/ header file:

$ grep cpython/ Include/Python.h#include "cpython/pydebug.h"#include "cpython/longintrepr.h"#include "cpython/odictobject.h"#include "cpython/funcobject.h"#include "cpython/classobject.h"#include "cpython/code.h"#include "cpython/cellobject.h"#include "cpython/initconfig.h"#include "cpython/genobject.h"#include "cpython/picklebufobject.h"#include "cpython/pytime.h"#include "cpython/context.h"#include "cpython/pyctype.h"#include "cpython/pyfpe.h"#include "cpython/tracemalloc.h"#include "cpython/optimizer.h"

Copy link
Member

@vstinnervstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM. Thanks for the different update and the doc.

@colesbury got an exception from@Yhg1s (3.13 release manager) to add this API to Python 3.13, god.

erlend-aasland reacted with rocket emoji
@colesburycolesbury merged commit3af7263 intopython:mainJun 20, 2024
38 checks passed
@miss-islington-app
Copy link

Thanks@colesbury for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@miss-islington-app
Copy link

Sorry,@colesbury, I could not cleanly backport this to3.13 due to a conflict.
Please backport usingcherry_picker on command line.

cherry_picker 3af7263037de1d0ef63b070fc7bfc2cf042eaebe 3.13

@colesburycolesbury deleted the gh-117511-pymutex branchJune 20, 2024 15:29
colesbury added a commit to colesbury/cpython that referenced this pull requestJun 20, 2024
colesbury added a commit to colesbury/cpython that referenced this pull requestJun 20, 2024
…ythonGH-117731)(cherry picked from commit3af7263)Co-authored-by: Sam Gross <colesbury@gmail.com>
colesbury added a commit to colesbury/cpython that referenced this pull requestJun 20, 2024
…ythonGH-117731)(cherry picked from commit3af7263)Co-authored-by: Sam Gross <colesbury@gmail.com>
@bedevere-app
Copy link

GH-120800 is a backport of this pull request to the3.13 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.13bugs and security fixes labelJun 20, 2024
colesbury added a commit that referenced this pull requestJun 20, 2024
@vstinner
Copy link
Member

Yeah, congrats!

@erlend-aasland
Copy link
Contributor

@colesbury got an exception from@Yhg1s (3.13 release manager) to add this API to Python 3.13, god.

That is very good news! Glad to see this land.

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

@ericsnowcurrentlyericsnowcurrentlyericsnowcurrently left review comments

@zoobazoobazooba left review comments

@Yhg1sYhg1sYhg1s left review comments

@erlend-aaslanderlend-aaslanderlend-aasland left review comments

@vstinnervstinnervstinner approved these changes

@encukouencukouAwaiting requested review from encukou

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

6 participants
@colesbury@vstinner@erlend-aasland@ericsnowcurrently@zooba@Yhg1s
[8]ページ先頭
©2009-2025 Movatter.jp