| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
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?
Sorry, something went wrong.
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 self. So args[-1] is meant to store self. See https://www.python.org/dev/peps/pep-0590/#py-vectorcall-arguments-offset |
Sorry, something went wrong.
|
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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request. |
Sorry, something went wrong.
|
@jdemeyer Wouldn't 1 | PY_VECTORCALL_ARGUMENTS_OFFSET always resolve to the same value in the case of this function though? The primary use for it seems to be calculating the offset when the number of args is unknown: or for the example provided in PEP 590: What is the purpose of using PY_VECTORCALL_ARGUMENTS_OFFSET when the the number of arguments for the function is static? As far as I can tell, the result of 1 | PY_VECTORCALL_ARGUMENTS_OFFSET will always be 1 any time _PyObject_CallOneArg() is utilized. Is this incorrect? 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. |
Sorry, something went wrong.
|
I have made the requested changes; please review again |
Sorry, something went wrong.
PY_VECTORCALL_ARGUMENTS_OFFSET is a constant flag, so of course 1 | PY_VECTORCALL_ARGUMENTS_OFFSET is a constant value. But that's not the point. The PY_VECTORCALL_ARGUMENTS_OFFSET flag is meant for the called function. It indicates that the value args[-1] may temporarily be changed as scratch space. That's the reason for the array _args of length two in this PR: _args[0] is the same as args[-1] which can be used as scratch space by the called function. |
Sorry, something went wrong.
|
@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 PyObject_Vectorcall had seemed redundant to me initially, but now I understand the reasoning. Thank you for taking the time to fully explain it, I certainly appreciate it. |
Sorry, something went wrong.
|
Thanks for the changes, the "approval" was a GitHub thing, I'm actually -0 on such large replacement PRs. |
Sorry, something went wrong.
|
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). |
Sorry, something went wrong.
|
@methane: Please replace # with GH- in the commit message next time. Thanks! |
Sorry, something went wrong.
|
I mean large replacement. Note that I did not protest against the changes in memoryobject.c. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
https://bugs.python.org/issue37483