Skip to content

bpo-33216: Clarify the documentation for CALL_FUNCTION_* #8338

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

Merged
merged 5 commits into from
Jul 19, 2018

Conversation

larryhastings
Copy link
Contributor

@larryhastings larryhastings commented Jul 19, 2018

This is my take on #6365. I spent some time reading the code so I could understand exactly what the bytecodes do, then figured out how best to describe it.

I put off 3.5.6rc1 to work on this. I'm going to take a few hours off; when I come back I'm going to tag 3.5.6rc1. If I can get a quick review before I do that, I'll merge this in to 3.5 and it'll go into 3.5.6rc1.

https://bugs.python.org/issue33216

@larryhastings
Copy link
Contributor Author

Note that these bytecodes changed dramatically for 3.6. So 3.5 is the latest version where these documentation changes are relevant. If this PR is accepted I will probably attempt to backport these changes to 3.4.

Calls a function, similarly to :opcode:`CALL_FUNCTION`.
*argc* represents the number of keyword and positional
arguments, identically to :opcode:`CALL_FUNCTION`.
The top of the stack contains an iterable object containing additional positional
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An iterable object containing additional positional arguments was at the top of the stack before 3.5, but it is below keyword arguments in 3.5. This issue was opened for documenting this subtle change. Even after reading the code it can be missed. Please add also the versionchanged directive.

The documentation for CALL_FUNCTION_VAR_KW needs the same change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By golly, you're right! Gosh that's subtle. I've corrected the corrections ;-) and pushed an update to the PR.

@serhiy-storchaka
Copy link
Member

It may be simpler to merge the current PR in 3.5, 3.4 and 2.7, and then create a separate PR which will document the 3.5 changes and fix bpo-33216.

@larryhastings
Copy link
Contributor Author

PR has been updated with the corrections pointed out by Serhiy. Thanks, Serhiy!

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind to add versionchanged directives for highlighting the changes?

@@ -1071,7 +1108,7 @@ instructions:

.. data:: hasconst

Sequence of bytecodes that have a constant parameter.
Sequence of bytecodes that have a constant argument.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "Sequence of bytecodes that access a constant." would be more accurate (if it is a correct English). argc is an index in the list of constants (the co_consts attribute of the code object).

Currently, in all maintained versions, hasconst contains only LOAD_CONST.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I only touched it because I was searching for "parameter", because nearly all the existing uses of the word "parameter" were wrong. But you're right, this one was completely wrong from the beginning. Will update the PR in a sec.

@larryhastings
Copy link
Contributor Author

I'm +0 on adding "versionchanged" directives. I don't know what changed in what version, so you would have to tell me. But also, I don't know who would find the "versionchanged" information useful. Everything changed in 3.6, and 3.7 is out now; how many people in the universe need to know about new changes to bytecodes in 3.5?

@serhiy-storchaka
Copy link
Member

CALL_FUNCTION_VAR and CALL_FUNCTION_VAR_KW was changed in 3.5.

Before 3.5 an iterable object containing additional positional arguments was above keyword arguments (as you wrote in the initial edition). In 3.5 it is below keyword arguments. This is a side effect of implementing PEP 448.

   .. versionchanged:: 3.5
      An iterable object containing additional positional arguments was above keyword arguments before Python 3.5.

This information would be useful for authors of third-party projects which analyze or generate bytecode.

@larryhastings
Copy link
Contributor Author

Done.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks!

@larryhastings larryhastings merged commit 76aa2c0 into python:3.5 Jul 19, 2018
@bedevere-bot
Copy link

@larryhastings: Please replace # with GH- in the commit message next time. Thanks!

@larryhastings larryhastings deleted the threefivebytecodedocfix branch July 19, 2018 23:35
@serhiy-storchaka
Copy link
Member

Do you mind to backport this change to 2.7 (without 3.5 specific changes) and clarify the documentation in 3.6+?

serhiy-storchaka pushed a commit to serhiy-storchaka/cpython that referenced this pull request Aug 16, 2018
…nGH-8338)

(cherry picked from commit 76aa2c0)

Co-authored-by: larryhastings <larry@hastings.org>
serhiy-storchaka pushed a commit to serhiy-storchaka/cpython that referenced this pull request Aug 16, 2018
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 17, 2018
…) (pythonGH-8784)

(cherry picked from commit 5e99b56)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 17, 2018
…) (pythonGH-8784)

(cherry picked from commit 5e99b56)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka added a commit that referenced this pull request Sep 17, 2018
…) (GH-8783)

(cherry picked from commit 76aa2c0)

Co-authored-by: larryhastings <larry@hastings.org>
miss-islington added a commit that referenced this pull request Sep 17, 2018
…H-8784)

(cherry picked from commit 5e99b56)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
miss-islington added a commit that referenced this pull request Sep 17, 2018
…H-8784)

(cherry picked from commit 5e99b56)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants