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.

Sign up for GitHub

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

Jump to bottom

bpo-45474: Exclude all of marshal.h if Py_LIMITED_API is defined #29061

Merged
merged 2 commits into from
Oct 20, 2021
Merged

bpo-45474: Exclude all of marshal.h if Py_LIMITED_API is defined #29061

merged 2 commits into from
Oct 20, 2021

Conversation

encukou
Copy link
Member

@encukou encukou commented Oct 19, 2021 •
edited by miss-islington
Loading

Also, reword the What's New messages: this doesn't change the limited API, it only brings the Py_LIMITED_API macro closer to the ideal of only allowing the limited API.

https://bugs.python.org/issue45474

Automerge-Triggered-By: GH:encukou

@encukou
Reword note on removing PyMarshal_* with Py_LIMITED_API
9a7098f 
These items were not part of the limited API, which is defined
in the docs (via Misc/stable_abi.txt).

The change brings the `Py_LIMITED_API` macro closer to the ideal
of only allowing things in the limited API.
@encukou
Exclude all of "marshal.h" when Py_LIMITED_API is defined.
7fc9eef 
Nothing in the file is listed as part of the limited API.
The symbols are not exported in the Windows stable ABI dlls.
The header is not included from <Python.h>.
@encukou encukou requested a review from vstinner October 19, 2021 14:41
@encukou encukou requested a review from tiran as a code owner October 19, 2021 14:41
vstinner
vstinner approved these changes Oct 19, 2021
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM. I wasn't sure, I made the least controversial change. Thanks for finishing the work ;-)

@encukou
Copy link
Member Author

encukou commented Oct 20, 2021

I wasn't sure

Well, you pinged me in the original PR, but merged before I got to reply. Next time, could you please wait?
I usually only work on to CPython one day a week. I know this is slow for you, but many other contributors have similar schedules.

@miss-islington
Copy link
Contributor

@encukou: Status check is done, and it's a success ✅ .

@miss-islington miss-islington merged commit 98fa3b5 into python:main Oct 20, 2021
@encukou encukou deleted the marshal_h branch October 20, 2021 09:32
@encukou
Copy link
Member Author

encukou commented Oct 20, 2021

Thanks for the review!

@vstinner
Copy link
Member

Well, you pinged me in the original PR, but merged before I got to reply. Next time, could you please wait?
I usually only work on to CPython one day a week. I know this is slow for you, but many other contributors have similar schedules.

I made a long series of changes on the C API (Include/ directory). GitHub doesn't support patch series, so I have to merge pull requests one by one. I already takes me between 30 min and 2 hour to merge a single PR, so I prefer to not have to wait for too long when I have a long serie.

Sure, I pinged you since you are now the limited C API / stable ABI expert! And it seems like it was worth it since you spotted more issues :-)

I was confident that using FILE* in the limited C API but I wasn't sure if further changes were needed. Usually, in case of doubt, I prefer to do nothing rather than making a mistake.

You made a second change to finish marshal API cleanup, IMO that's the way to go ;-) Nice team work!

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

@vstinner vstinner vstinner approved these changes

@tiran tiran Awaiting requested review from tiran

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

Successfully merging this pull request may close these issues.
5 participants
@encukou @miss-islington @vstinner @the-knights-who-say-ni @bedevere-bot