FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add guidelines for C API with output parameters by erlend-aasland · Pull Request #1128 · python/devguide · GitHub

Add guidelines for C API with output parameters - #1128

Closed
erlend-aasland wants to merge 6 commits into
python:mainfrom
erlend-aasland:c-api/guidelines-output-params
Closed

Add guidelines for C API with output parameters#1128
erlend-aasland wants to merge 6 commits into
python:mainfrom
erlend-aasland:c-api/guidelines-output-params

Conversation

erlend-aasland commented Jun 26, 2023
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor

Comment thread developer-workflow/c-api.rst Outdated
Comment thread developer-workflow/c-api.rst Outdated
erlend-aasland linked an issue Jun 26, 2023 that may be closed by this pull request

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, I just left some comments

Comment thread developer-workflow/c-api.rst Outdated
PyFoo_Bar(PyObject **out)
{
PyObject *value;
int rc = foo_bar(&value);

vstinner Jun 26, 2023
edited by terryjreedy
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM.

Copy link
Copy Markdown
Contributor Author

See also python/cpython#108797.

vstinner commented Sep 6, 2023

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
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 commented Sep 6, 2023

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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 commented Sep 7, 2023

Copy link
Copy Markdown
Member

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

I concur with @erlend-aasland on that.

vstinner commented Sep 7, 2023

Copy link
Copy Markdown
Member

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 added the DO-NOT-MERGE Do not merge this PR label Oct 10, 2023

Copy link
Copy Markdown
Contributor Author

I'll leave this to the C API workgroup.

erlend-aasland deleted the c-api/guidelines-output-params branch October 11, 2023 21:03
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DO-NOT-MERGE Do not merge this 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


Back | FazBrowse Home | New Git URL