Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 33.9k
gh-98586: Add vector call APIs to the Limited API#98587
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
wjakob commented Oct 24, 2022 • edited by bedevere-bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-bot
Uh oh!
There was an error while loading. Please reload this page.
abc4964 to 85d09f5Compareerlend-aasland commented Oct 24, 2022
@markshannon might be interested in reviewing this as the author of PEP 590. |
erlend-aasland left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add tests to Modules/_testcapimodule/vectorcall_limited.c and also adjust the NEWS entry to use Sphinx directives for linking to the relevant parts of the C-API docs and PEP-590?
85d09f5 to 8732fb0Comparewjakob commented Oct 24, 2022
@erlend-aasland Done. |
8732fb0 to 9934529Compareerlend-aasland commented Oct 24, 2022
What is the reason for not adding |
wjakob commented Oct 24, 2022
It seemed like sort of an implementation detail. I am happy to add it (& tests) if there is consensus to do so. |
erlend-aasland commented Oct 24, 2022
I've never had use for it myself, so I have no opinion about this; I was just curious. Let's hear what the others say when they chime in. |
Misc/NEWS.d/next/Core and Builtins/2022-10-24-10-30-30.gh-issue-98586.Tha5Iy.rst Outdated Show resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
markshannon commented Oct 25, 2022
Two questions. First questionWhy include Second questionThe vectorcall API has two parts.
Any plans to add the ability to declare classes as having "vector callable" instances? |
wjakob commented Oct 25, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Thanks for the review @markshannon.
I am happy to hear it. I have never found use for this function myself. I included in the PR for reasons of symmetry, and because it was prominently advertised in the docs next to
I think that this was already done in PR #93274. Please let me know if I misunderstood. Finally, do you have any thoughts on |
markshannon commented Oct 25, 2022
One other question. Why add This is a sketch and is probably missing some error handling and/or incref/decrefs. PyObject*PyObject_VectorcallMethod( PyObject*name, PyObject*const*args, size_tnargsf, PyObject*kwnames){/* Error if nargs < 1 */PyObject*obj=args[0]; PyObject*callable=LoadMethod(obj, name, &args[0]); /* TO DO --- Handle error if callable == NULL */if (args[0] ==NULL){/* No self */returnPyObject_Vectorcall(callable, args+1, (nargsf-1) | PY_VECTORCALL_ARGUMENTS_OFFSET, kwnames)} returnPyObject_Vectorcall(callable, args, nargsf, kwnames)} |
markshannon commented Oct 25, 2022
I'd rather not add it. |
wjakob commented Oct 25, 2022
It is convenient to use a 1-function-does-it-all interface like Altogether, it sounds to me more like a general question about why |
wjakob commented Oct 26, 2022
Is this okay @markshannon? (with a goto to avoid lots of I have made the requested changes; please review again |
bedevere-bot commented Oct 26, 2022
Thanks for making the requested changes! @erlend-aasland, @markshannon: please review the changes made to this pull request. |
wjakob commented Oct 26, 2022
The |
AlexWaygood commented Oct 26, 2022
It's a known issue that will be fixed by #98704 |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
encukou left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also have a nitpick, otherwise LGTM.
I'll push a commit to your branch to avoid another round of review ping-pong.
| * (Empty list left here as template/example, since using | ||
| * PyModule_AddFunctions isn't very common.) | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment can go away now.
encukou commented Oct 27, 2022
Thank you for the PR! |
wjakob commented Oct 27, 2022
Awesome, many thanks for your help in getting it merged! |
Expose the facilities for making vector calls through Python's limited API.
wjakob commented Nov 3, 2022
I realized that I forgot to add an entry to |
markshannon commented Nov 3, 2022
Open a new PR please. |
PR python#98587 addressing issue python#98586 lacked a "what's new" entry. While making those changes, I noticed an inconsistency in how ``PY_VECTORCALL_ARGUMENTS_OFFSET`` is declared in the underlying Sphinx markup when compared to other macro constants like ``Py_TPFLAGS_HAVE_VECTORCALL``. This PR fixes that as well, which should connect a few currently broken cross-references
This PR constains tentative changes needed to expose the facilities for making PEP-590-style vector calls through Python's limited API.