bpo-37483: add _PyObject_CallOneArg() function#14558
Conversation
47ec7ac to
b4129e2
Compare
There was a problem hiding this comment.
@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