-
-
Notifications
You must be signed in to change notification settings - Fork 32k
bpo-37483: add _PyObject_CallOneArg() function #14558
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
Conversation
47ec7ac
to
b4129e2
Compare
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.
@jdemeyer On lines 145 and 146 of abstract.h:
return _PyObject_Vectorcall(func, args, 1 | PY_VECTORCALL_ARGUMENTS_OFFSET, NULL);
What is the purpose of using the constant PY_VECTORCALL_ARGUMENTS_OFFSET
in a function that is made specifically for single arguments? Unless I am misunderstanding something, return _PyObject_Vectorcall(func, args, 1, NULL);
would effectively have the same functionality. Is there any circumstance where this function is used and 1 | PY_VECTORCALL_ARGUMENTS_OFFSET
does not result in 1
?
It's meant for optimizing calls to bound methods. If you're calling a bound method with 1 argument, you're effectively calling a function with 2 arguments when you add |
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
@jdemeyer Wouldn't or for the example provided in PEP 590: What is the purpose of using Also, apologies for the questions if these answers are fairly obvious. I have some understanding of C fundamentals, but I'm significantly more experienced with Python. I'm just trying to get a solid understanding of both so that I can be more useful as a contributor. |
|
The |
@jdemeyer Oh that makes sense, I see. I hadn't realized the purpose of it within this function was solely to be an indicating flag, in general I'm far more used to working on much smaller scale projects and codebases where almost everything has a specific functional purpose. Seeing the two constant values being used for the nargs argument of Thank you for taking the time to fully explain it, I certainly appreciate it. |
Thanks for the changes, the "approval" was a GitHub thing, I'm actually -0 on such large replacement PRs. |
It's not doing replacement for the sake of replacement. As I explained on bpo-37483, it gives a nice performance gain (in terms of runtime and stack usage). |
@methane: Please replace |
I mean large replacement. Note that I did not protest against the changes in |
https://bugs.python.org/issue37483