Skip to content

gh-116646: Add limited C API support to AC fildes converter #116769

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

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Mar 13, 2024

Add tests on the "fildes" converter to _testclinic_limited.
@vstinner
Copy link
Member Author

@serhiy-storchaka @erlend-aasland: Ok, this approach should be the least intrusive and the least controversial. It only adds support for the limited C API, but keep the code for the internal C API using converter.

In the worst case, it generates internal C API code for the limited C API, but we can revisit this corner case later.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Why bother with limited_capi? It is okay to use PyObject_AsFileDescriptor unconditionally.

@vstinner
Copy link
Member Author

Why bother with limited_capi? It is okay to use PyObject_AsFileDescriptor unconditionally.

You explained to me that all converters must support the PyArg_ParseTuple() fallback, here using O& and _PyLong_FileDescriptor_Converter(), when parse_arg() cannot be used.

I prefer to move slowly, step by step, when touching Argument Clinic which is fragile and has a small test suite. This change adds a test on the "fildes" converter for the limited C API.

@serhiy-storchaka
Copy link
Member

The fallback is still here if you do not remove converter = '_PyLong_FileDescriptor_Converter'.

@serhiy-storchaka
Copy link
Member

I prefer to not add an indentation and the limited_capi check. The fallback is still used in a number of modules.

@vstinner
Copy link
Member Author

vstinner commented Mar 14, 2024

The fallback is still here if you do not remove converter = '_PyLong_FileDescriptor_Converter'.

Ok, let me elaborate. I tried to move code to converter_init(). The problem is that the logic to decide if an #include must be added or not doesn't work well. It's because converter_init() is always called: calling add_include() means that the include is always added, even if it's not used. I plan to write a follow-up PR to adjust this.

@vstinner vstinner merged commit d402872 into python:main Mar 14, 2024
@vstinner vstinner deleted the clinic_fd2 branch March 14, 2024 09:28
@vstinner
Copy link
Member Author

Merged, thanks for the review @serhiy-storchaka.

@bedevere-bot

This comment was marked as off-topic.

@erlend-aasland
Copy link
Contributor

LGTM; thanks.

@erlend-aasland
Copy link
Contributor

[...] when touching Argument Clinic which is fragile and has a small test suite.

You keep repeating this, but the latter part is not true. Please keep to the facts, and don't invent FUD about the Argument Clinic test suite.

@vstinner
Copy link
Member Author

Your right, AC has a large test suite today, and mypy is a great help. Sorry.

vstinner added a commit to vstinner/cpython that referenced this pull request Mar 20, 2024
…thon#116769)

Add tests on the "fildes" converter to _testclinic_limited.
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
…thon#116769)

Add tests on the "fildes" converter to _testclinic_limited.
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
…thon#116769)

Add tests on the "fildes" converter to _testclinic_limited.
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.

4 participants