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

Add guidelines for C API with output parameters #1128

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jun 26, 2023

@erlend-aasland erlend-aasland linked an issue Jun 26, 2023 that may be closed by this pull request
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 left some comments

developer-workflow/c-api.rst Outdated Show resolved Hide resolved
PyFoo_Bar(PyObject **out)
{
PyObject *value;
int rc = foo_bar(&value);
Copy link
Member

@vstinner vstinner Jun 26, 2023

Choose a reason for hiding this comment

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

Maybe you could mimic a legacy API call which returns NULL if not found and on error, and call PyErr_Occurred(). So the difference with the two APIs is even more obvious?

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.

@erlend-aasland
Copy link
Contributor Author

See also python/cpython#108797.

@vstinner
Copy link
Member

vstinner commented Sep 6, 2023

Do you need a different review? Or are you waiting for something?

@erlend-aasland
Copy link
Contributor Author

Do you need a different review? Or are you waiting for something?

I think @encukou wants to wait until the sprint before any C API guidelines are updated in any kind of way. Correct me if I'm wrong, Petr.

@vstinner
Copy link
Member

vstinner commented Sep 6, 2023

I think @encukou wants to wait until the sprint before any C API guidelines are updated in any kind of way. Correct me if I'm wrong, Petr.

Oh, I wasn't aware of that.

@serhiy-storchaka
Copy link
Member

Some converters, like _PyEval_SliceIndex(), are designed so that they do not set the output parameter if the argument is None. It allows to set the default value to what you want: 0, -1, PY_SSIZE_T_MAX or Py_SIZE(self). It is the only way, because the signature of such converters is fixed, and they cannot take other arguments.

@erlend-aasland
Copy link
Contributor Author

Some converters, like _PyEval_SliceIndex(), are designed so that they do not set the output parameter if the argument is None. It allows to set the default value to what you want: 0, -1, PY_SSIZE_T_MAX or Py_SIZE(self). It is the only way, because the signature of such converters is fixed, and they cannot take other arguments.

As I commented on python/cpython#108797, I think the guideline should care only about the general case, not the special case. There will be deviant APIs once in a while; that's ok.

BTW, _PyEval_SliceIndex is exposed through Python.h, but I guess it is considered an internal and/or private API since it is not documented and it is prefixed with an underscore. The guidelines are for public API.

@vstinner
Copy link
Member

vstinner commented Sep 7, 2023

I think the guideline should care only about the general case, not the special case.

I concur with @erlend-aasland on that.

@vstinner
Copy link
Member

vstinner commented Sep 7, 2023

FYI there is one public converter :-) PyUnicode_FSConverter().

I tried to move other private ones to the internal C API, but it's quite complicated: python/cpython#106320 (comment) I decided to give up in Python 3.13. I prefer to wait to see what's going on with the limited C API.

@willingc willingc added the status-do not merge Do not merge the PR label Oct 10, 2023
@erlend-aasland
Copy link
Contributor Author

I'll leave this to the C API workgroup.

@erlend-aasland erlend-aasland deleted the c-api/guidelines-output-params branch October 11, 2023 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status-do not merge Do not merge the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

C API: Add guidelines for C APIs with output params
4 participants