Skip to content

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

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