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

bpo-36974: expand call protocol documentation #13844

Merged
merged 7 commits into from
Nov 12, 2019

Conversation

jdemeyer
Copy link
Contributor

@jdemeyer jdemeyer commented Jun 5, 2019

CC @encukou

I'm also adding Petr Viktorin as contributor for vectorcall in the "what's new" section.

https://bugs.python.org/issue36974

Automerge-Triggered-By: @encukou

Automerge-Triggered-By: @encukou

@jdemeyer
Copy link
Contributor Author

jdemeyer commented Jun 5, 2019

Note that the first commit just moves the existing documentation, the second commit actually makes the changes.

@jdemeyer
Copy link
Contributor Author

jdemeyer commented Jun 8, 2019

@vstinner: I moved your doc fixes from #13890 here.

@jdemeyer
Copy link
Contributor Author

Rebased to latest master, squashed most commits. The commit simply moving the documentation and the one coming from @vstinner are still separate.

Can this be reviewed please? It's a documentation-only patch, so it shouldn't be controversial. There are other PRs open for review or planned that will affect the documentation.

Please also add the labels skip news and needs backport to 3.8.

CC @methane

@jdemeyer
Copy link
Contributor Author

I give up

@jdemeyer
Copy link
Contributor Author

I'm rebooting this PR since I don't expect any further changes to the call API. As before, there are two commits: one to create Doc/c-api/call.rst and one making actual modifications.

Doc/c-api/call.rst Outdated Show resolved Hide resolved
Doc/c-api/call.rst Outdated Show resolved Hide resolved
Doc/c-api/call.rst Outdated Show resolved Hide resolved
Doc/c-api/call.rst Outdated Show resolved Hide resolved
Doc/c-api/call.rst Outdated Show resolved Hide resolved
Doc/c-api/call.rst Outdated Show resolved Hide resolved
Doc/c-api/call.rst Outdated Show resolved Hide resolved
Doc/c-api/call.rst Outdated Show resolved Hide resolved
Doc/c-api/call.rst Outdated Show resolved Hide resolved
Doc/c-api/exceptions.rst Outdated Show resolved Hide resolved
@encukou
Copy link
Member

encukou commented Sep 9, 2019

I hope to look at this at this week's sprint. To make it faster, I plan to push changes directly to your branch, @jdemeyer. Let me know if you want more back-and-forth.

@jdemeyer
Copy link
Contributor Author

jdemeyer commented Sep 9, 2019

To make it faster, I plan to push changes directly to your branch, @jdemeyer.

Sure. But please give me a final look before merging :-)

jdemeyer and others added 2 commits September 9, 2019 22:52
- Repeat the tp_call signature, for clarity
- Warn that vectorcall API is provisional
- Make the existing warning smaller, but move the explanation
  up a bit
- Group calling API together and provide a summary table
- Put smaller vectorcall-related sections under vectorcall
- Minor rewordings
@encukou
Copy link
Member

encukou commented Sep 10, 2019

Here are my changes. Sorry for the big diff; I can't do the rewordings and reorganization separately and I don't think it's practical to split afterwards.

@encukou
Copy link
Member

encukou commented Sep 11, 2019

@jdemeyer, are you OK with this?

@jdemeyer
Copy link
Contributor Author

I don't mind adding

.. warning::

   The vectorcall API is provisional and expected to become public in
   Python 3.9, with a different name and, possibly, changed semantics.
   If you use the function, plan for updating your code for Python 3.9.

to the general section about vectorcall, but this text refers to a specific function, so it should be reworded.

@jdemeyer
Copy link
Contributor Author

I really like the table 👍 but I have a few comments:

  1. PyObject_CallOneArg -> _PyObject_CallOneArg (missing underscore)
  2. PyObject_CallFunction and _PyObject_CallMethodNoArgs are missing
  3. I would write "vectorcall" under "args" in the _PyObject_FastCallDict() line because the handling of "args" really is the same as _PyObject_Vectorcall.
  4. For methods, you always write char* but in some cases it's really a Python object with the name instead of a char*.

@jdemeyer
Copy link
Contributor Author

Other than that, looks good!

- Adjust vectorcall warning to not talk about a single function
- Add underscore to _PyObject_CallOneArg
- Include PyObject_CallFunction and _PyObject_CallMethodNoArgs
- Summarize _PyObject_FastCallDict's args handling as "vectorcall"
- Use "name" rather chan "char*" when the name is a Python object
@encukou
Copy link
Member

encukou commented Sep 12, 2019

Alright. The remaining thing is to address the underscore-prefixed functions. Do we want to expose those in Python 3.9, or keep them private (in which case they shouldn't be documented externally)?

@jdemeyer
Copy link
Contributor Author

OK, for me this can be merged.

@jdemeyer
Copy link
Contributor Author

Now about the API: so far, I have implemented vectorcall for a few classes in CPython itself and in Cython and I haven't seen any reason to change the API. I also think that all functions which are documented in this PR are useful enough to be public API.

What that means for the naming of these functions should really be discussed more openly, outside of this PR. Some core devs are very reluctant to add anything to the public API. And once you make a function like _PyObject_CallOneArg public, the next question is whether it should be part of the limited API. There are many cases where we have a fast function outside of the limited API and a slower wrapper in the limited API, for example PyList_GetItem and PyList_GET_ITEM or PyObject_CallNoArgs and _PyObject_CallNoArg. That might be a good model for some of these functions.

@encukou
Copy link
Member

encukou commented Sep 13, 2019

Do you have any notes on how useful these are in Cython? (Or other projects)

@miss-islington
Copy link
Contributor

@jdemeyer: Status check is done, and it's a success ✅ .

@miss-islington
Copy link
Contributor

Sorry, I can't merge this PR. Reason: Required status check "Azure Pipelines PR" is expected..

@vstinner
Copy link
Member

Good idea to put all call documentation at the same place ;-)

@miss-islington
Copy link
Contributor

@jdemeyer: Status check is done, and it's a success ✅ .

@miss-islington miss-islington merged commit 9a13a38 into python:master Nov 12, 2019
jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
CC @encukou 

I'm also adding Petr Viktorin as contributor for vectorcall in the "what's new" section.


https://bugs.python.org/issue36974



Automerge-Triggered-By: @encukou

Automerge-Triggered-By: @encukou
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
CC @encukou 

I'm also adding Petr Viktorin as contributor for vectorcall in the "what's new" section.


https://bugs.python.org/issue36974



Automerge-Triggered-By: @encukou

Automerge-Triggered-By: @encukou
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants