-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
[3.5] bpo-33216: Correct the doc of CALL_FUNCTION_VAR and CALL_FUNCTION_VAR_KW. #6365
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
The order of the elements in the stack was incorrect. It seems that, in general (for all CALL_FUNCTION opcodes), the correct order is (from bottom to top): function positional arguments [star arg tuple] keyword arguments [keyword dictionary] CALL_FUNCTION[_..] Fixes [bug 33216](https://bugs.python.org/issue33216).
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. We also demand... A SHRUBBERY! Thanks again to your contribution and we look forward to looking at it! |
@mvaled, please merge this branch with master. And maybe add the |
@serhiy-storchaka I put the I'm not a C developer (I can write some, but I don't do it often). Since I'd have to resolve all conflicts, I propose to first merge into '3.6', then to '3.7' and finally to 'master'. This way I could solve the conflicts incrementally and see if some things added to 3.5 are applicable to other branches. What do you think? |
I said "merge with master"? My bad, I missed that this PR is against 3.5. It should be merged with 3.5. I can't merge this pull request because AppVeyor requires synchronizing the branch with the upstream branch (3.5 in this case) for running tests, and passing integration tests is required. I can't update this branch via the web interface because you have not granted a write access to your repository. Only you can merge your branch with the upstream 3.5. |
@serhiy-storchaka I've just added you as a collaborator for that repository. I really have no clue how does AppVeyor works. |
When you say "upstream" do you mean this repository or another? |
Ah, I just have no permissions of merging into the 3.5 branch! @larry, please merge this documentation fix. Third-party tools support CPython bytecode with the delay and they can miss this corner case in 3.5 (in 3.6 all was changed again). Yes, I meant this repository. |
I'm closing this pull request. First, as mentioned, this should be against master, not against 3.5. We could backport this to 3.5, but the best place to start would be master and then work back from there. Second, the original documentation is incorrect and badly written, and the update does little to correct these deficiencies. I can't get excited about that. We don't push a "keyword arguments dictionary", nor do we push a "variable-arguments tuple". We don't push a dictionary, we don't push a dict, and "variable-arguments" is not a meaningful term. I would be interested in this PR (for master) if it actually accurately described what is being pushed on the stack for these two opcodes. |
This can't be against master, because these opcodes were removed in 3.6. 3.5 is the only version that needs a doc fix. |
Use 'var-keyword' and 'var-positional' to better specify the items in the stack. Describe each opcode by stating the difference with CALL_FUNCTION.
Hi @larryhastings, I've just pushed a commit that, I think, addresses your objections. |
It seems that github does not update the commits in the PR, because is closed. |
@larryhastings, sorry, my English is so bad, that I don't understand what is wrong with this PR. Do you like to fix the grammar or suggest a better wording? |
The problem is, the author of the documentation fundamentally doesn't understand what the bytecodes are doing at a technical level, and their grasp of the relevant technical terms is poor. So their changes don't improve the documentation--quite the opposite, so far their attempts have made it worse. The current attempt describes the use of "var-positional arguments", an imaginary concept which the author has made up. First, there are three possible types of parameters: positional-only, keyword-or-positional, and keyword-only. Second, parameters are only relevant on the callee side. This documentation is for the bytecodes, which deals with strictly the caller side, and only deals with arguments, not parameters. And the caller side only has two types of arguments: "positional" and "keyword". I appreciate the person creating the PR request is trying to make Python better. Unfortunately, it's clear that they don't understand what they are doing with respect to this PR. I'm going to close this PR (again), and my hope is that it is not re-opened, as I think the creator of the PR has a lot to learn before they can contribute meaningfully in this area. I may, however, create my own PR to address this minor deficiency in the documentation. If so, I'll mention it in a comment on this PR. |
Thank you! |
I wrote my own version of this PR, which you can find at #8338. |
The order of the elements in the stack was incorrect. It seems that, in general (for all CALL_FUNCTION opcodes), the correct order is (from bottom to top):
Fixes bug 33216.
https://bugs.python.org/issue33216