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-40422: move _Py_closerange to core #22680

Merged
merged 3 commits into from
Oct 13, 2020
Merged

bpo-40422: move _Py_closerange to core #22680

merged 3 commits into from
Oct 13, 2020

Conversation

kevans91
Copy link
Contributor

@kevans91 kevans91 commented Oct 12, 2020 •
edited by bedevere-bot
Loading

This API is relatively lightweight and organizationally, given that it's
used by multiple modules, it makes sense to move it to fileutils.

https://bugs.python.org/issue40422

@kevans91
bpo-40422: move _Py_closerange to core
0f42664 
This API is relatively lightweight and organizationally, given that it's
used by multiple modules, it makes sense to move it to fileutils.
@gpshead gpshead requested a review from vstinner October 13, 2020 00:34
@gpshead gpshead self-assigned this Oct 13, 2020
@gpshead
Copy link
Member

gpshead commented Oct 13, 2020

LGTM. Just leaving time for @vstinner to confirm this is what he had in mind. Victor: feel free to merge.

vstinner
vstinner reviewed Oct 13, 2020
View reviewed changes
@vstinner
Copy link
Member

I had originally started going that route, but either subprocess of posixmodule (I want to say subprocess) doesn't get built in the needed context for that (from memory, that header required some kind of pycore define).

It is made on purpose: the internal C API should not be used by 3rd party C extensions.

You can modify setup.py and Modules/Setup to define the Py_BUILD_CORE_BUILTIN macro. There are many examples in these files.

@kevans91
Move _Py_closerange to the proper spot for internal API
cbf8678 
Requires making sure that _posixsubprocess is compiled with the appropriate
Py_BUIILD_CORE_BUILTIN macro.
@kevans91 kevans91 requested a review from gpshead as a code owner October 13, 2020 12:39
@kevans91
Copy link
Contributor Author

Sure, easy enough; I've moved the declaration and defined Py_BUILD_CORE_{BUILTIN,MODULE} as appropriate. I made sure to test with Modules/Setup including the module as well.

vstinner
vstinner approved these changes Oct 13, 2020
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 just have a minor coding style suggestion.

@kevans91 @vstinner
Incorporate indentation change
5085211 
Co-authored-by: Victor Stinner <vstinner@python.org>
@kevans91
Copy link
Contributor Author

Thanks for being patient with me here~ still getting used to the organization here, since I've only done drive-by optimization work on CPython.

@vstinner vstinner merged commit 7992579 into python:master Oct 13, 2020
@vstinner
Copy link
Member

Merged, thanks! Python will now be really efficient to close file descriptors on recent FreeBSD versions! :-D

xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020
@kevans91
bpo-40422: Move _Py_closerange to fileutils.c (pythonGH-22680)
0eb3a0c 
This API is relatively lightweight and organizationally, given that it's
used by multiple modules, it makes sense to move it to fileutils.

Requires making sure that _posixsubprocess is compiled with the appropriate
Py_BUIILD_CORE_BUILTIN macro.
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@kevans91 @adorilson
bpo-40422: Move _Py_closerange to fileutils.c (pythonGH-22680)
68f67f5 
This API is relatively lightweight and organizationally, given that it's
used by multiple modules, it makes sense to move it to fileutils.

Requires making sure that _posixsubprocess is compiled with the appropriate
Py_BUIILD_CORE_BUILTIN macro.
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

@gpshead gpshead Awaiting requested review from gpshead

Assignees

@gpshead gpshead

Labels
skip news
Projects
None yet
Milestone
No milestone
Development

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