Skip to content

[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

Closed
wants to merge 4 commits into from

Conversation

mvaled
Copy link

@mvaled mvaled commented Apr 3, 2018

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

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).
@the-knights-who-say-ni
Copy link

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!

@serhiy-storchaka
Copy link
Member

@mvaled, please merge this branch with master.

And maybe add the versionchanged directives for CALL_FUNCTION_VAR and CALL_FUNCTION_VAR_KW?

@mvaled
Copy link
Author

mvaled commented Apr 30, 2018

@serhiy-storchaka I put the versionchanged marks. However, while merging my branch '3.5' into master (at 5a49ca6) there are a lot of conflicts and modifications in files other than dis.rst.

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?

@serhiy-storchaka
Copy link
Member

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.

@mvaled
Copy link
Author

mvaled commented May 1, 2018

@serhiy-storchaka I've just added you as a collaborator for that repository. I really have no clue how does AppVeyor works.

@mvaled
Copy link
Author

mvaled commented May 4, 2018

When you say "upstream" do you mean this repository or another?

@serhiy-storchaka
Copy link
Member

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.

@serhiy-storchaka serhiy-storchaka changed the title bpo-33216: Correct the doc of CALL_FUNCTION_VAR and CALL_FUNCTION_VAR_KW. [3.5] bpo-33216: Correct the doc of CALL_FUNCTION_VAR and CALL_FUNCTION_VAR_KW. May 5, 2018
@larryhastings
Copy link
Contributor

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.

@serhiy-storchaka
Copy link
Member

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.
@mvaled
Copy link
Author

mvaled commented May 25, 2018

Hi @larryhastings,

I've just pushed a commit that, I think, addresses your objections.

@mvaled
Copy link
Author

mvaled commented May 25, 2018

It seems that github does not update the commits in the PR, because is closed.

@serhiy-storchaka
Copy link
Member

@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?

@larryhastings
Copy link
Contributor

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.

@serhiy-storchaka
Copy link
Member

I may, however, create my own PR to address this minor deficiency in the documentation.

Thank you!

@larryhastings
Copy link
Contributor

I wrote my own version of this PR, which you can find at #8338.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting merge docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants