Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-117873: Use positional-only parameters in _posixshmem #118012

Merged
merged 1 commit into from
May 10, 2024

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Apr 17, 2024

  • shm_unlink() parameter becomes positional-only.
  • shm_open() first parameter (path) becomes positional-only, the two following parameters remain positional-or-keyword.

@vstinner
Copy link
Member Author

I convert this PR to a draft until the main branch becomes Python 3.14.

@vstinner vstinner marked this pull request as ready for review May 8, 2024 16:35
@vstinner
Copy link
Member Author

vstinner commented May 8, 2024

The main branch is now Python 3.14. I mark the PR as ready for review.

@erlend-aasland @corona10: Do you want to review it?

* shm_unlink() parameter becomes positional-only.
* shm_open() first parameter (path) becomes positional-only,
  the two following parameters remain positional-or-keyword.
@vstinner vstinner merged commit 7cc5e81 into python:main May 10, 2024
34 checks passed
@vstinner vstinner deleted the posixshmem branch May 10, 2024 10:04
@erlend-aasland
Copy link
Contributor

This PR does not address the concerns of the issue: the METH_VARARGS calling convention is still used.

@vstinner
Copy link
Member Author

shm_unlink() now uses METH_O.

Using positional-only arguments for shm_open() sounds like a bad API, that's not my intent. Do you want to change its API?

@erlend-aasland
Copy link
Contributor

shm_unlink() now uses METH_O.

Correct; I was talking about shm_open. Sorry for not making that clear.

Using positional-only arguments for shm_open() sounds like a bad API, that's not my intent. Do you want to change its API?

I think that particular change should be reverted.

@@ -32,6 +32,7 @@ module _posixshmem
/*[clinic input]
_posixshmem.shm_open -> int
path: unicode
/
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, why not use the / [3.16] syntax? These are user-visible changes, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a private module: _posixshmem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then why the concern about making all the params positional-only?

@erlend-aasland
Copy link
Contributor

Using positional-only arguments for shm_open() sounds like a bad API, that's not my intent. Do you want to change its API?

If the API is not exposed, why care about it being positional-only? :)

@vstinner
Copy link
Member Author

Then why the concern about making all the params positional-only?
If the API is not exposed, why care about it being positional-only? :)

It's to make the API closer to open() API. It's not related to #117873 directly.

@erlend-aasland
Copy link
Contributor

It's to make the API closer to open() API. It's not related to #117873 directly.

If it is an internal and unexposed API; why care if it is identical to open()? Does it matter?

@vstinner
Copy link
Member Author

I wrote #118901 to revert shm_open() change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants