gh-96461: clarify the meaning of the oparg for CACHE and COPY opcode #96462

MatthieuDartiailh · 2022-08-31T11:24:35Z

This could be backported to Python 3.11

Issue: CACHE and SWAP documentation is vague with the meaning of the oparg #96461

…s doc

Doc/library/dis.rst

…description

iritkatriel · 2022-09-01T13:20:49Z

Doc/library/dis.rst

@@ -398,13 +401,17 @@ The Python compiler currently generates the following bytecode instructions.
   Push the *i*-th item to the top of the stack. The item is not removed from its
   original location.

+   The stack is indexed from 1, so COPY 1 will copy TOS.


Maybe instead of adding this, we could define TOS(i) at the top along with TOS1, TOS2 etc, and then reword the definition here to use those.

I tried something along those lines in my last commit let me know if you like it better this way.

iritkatriel · 2022-09-01T14:18:06Z

Doc/library/dis.rst

@@ -382,6 +382,10 @@ The Python compiler currently generates the following bytecode instructions.

 **General instructions**

+In the following, TOS(i) refer to the *i*-th item on the stack which is index from 1.


Suggested change

In the following, TOS(i) refer to the *i*-th item on the stack which is index from 1.

In the following, TOS(i) refers to the *i*-th item on the stack which is indexed from 1.

iritkatriel · 2022-09-01T14:22:18Z

Doc/library/dis.rst

@@ -382,6 +382,10 @@ The Python compiler currently generates the following bytecode instructions.

 **General instructions**

+In the following, TOS(i) refer to the *i*-th item on the stack which is index from 1.
+We also use as shorthand TOS = TOS(1) which is the top-of-stack.
+TOS1, TOS2, TOS3 are the second, third and fourth items on the stack, respectively.


It's a bit unfortunate that TOS1 = TOS(2), TOS2 = TOS(3), etc.

@markshannon - would you like to change this?

This was another reason for my previous formulation that avoided ro shed too much light on this.

Generally we count from one for stack items, so I would like to change it. Ideally drop TOS1 etc, all together.
The current explanation is not explicit about what is popped or not, so the shorthand is not helpful.

markshannon · 2022-10-03T19:25:29Z

Doc/library/dis.rst

@@ -395,15 +399,15 @@ The Python compiler currently generates the following bytecode instructions.

 .. opcode:: COPY (i)

-   Push the *i*-th item to the top of the stack. The item is not removed from its
+   Push TOS(i) to the top of the stack. The item is not removed from its


Maybe "Push a copy of ith item (counting from 1) to the top of the stack ..."

markshannon · 2022-10-03T19:33:59Z

Doc/library/dis.rst

   original location.

   .. versionadded:: 3.11


 .. opcode:: SWAP (i)

-   Swap TOS with the item at position *i*.
+   Swap TOS with TOS(i).


Maybe "Swap the first and ith item on the stack (counting from 1).

MatthieuDartiailh · 2022-10-05T07:31:51Z

I can rephrase everything to not use TOS(i) and repeat that the stack is indexed from 1 in each relevant place however they are quite a few occurrences (many more than what this PR currently does).

@iritkatriel do you agree with @markshannon on this ?

iritkatriel · 2022-10-05T07:52:09Z

I can rephrase everything to not use TOS(i) and repeat that the stack is indexed from 1 in each relevant place however they are quite a few occurrences (many more than what this PR currently does).

@iritkatriel do you agree with @markshannon on this ?

I agree that we don't need to have both TOS(i) and TOSi, I think removing the TOSis is the smaller change, amirite?

@markshannon - were you suggesting to add "(counting from 1)" on each place where TOS(i) is used? Or just once where it's defined or used for the first time?

MatthieuDartiailh · 2022-10-05T08:33:12Z

Removing the TOSi should be a smaller change yes and would not entail any large duplication.

MatthieuDartiailh · 2022-10-06T19:43:03Z

I went ahead and replaced TOS by TOS(1)

I am not sure this a clear win for the reader TBH. Let me know what you think.

sweeneyde · 2022-10-07T00:25:54Z

I don't know if this has already been considered, but one approach might be to eliminate "TOS" terminology altogether and instead refer to stack[-1], stack[-2], stack.pop(), etc., borrowing the well-understood semantics of lists.

POP_TOP becomes "Remove stack[-1], i.e., do stack.pop()"
COPY becomes stack.append(stack[-i]).
SWAP becomes stack[-1], stack[-i] = stack[-i], stack[-1]
UNARY_POSITIVE becomes stack[-1] = +stack[-1]
BINARY_SUBSCR becomes right = stack.pop(); left = stack.pop(); stack.append(left[right]), or similar.

What do you think?

MatthieuDartiailh · 2022-10-07T07:15:06Z

That may lead to clearer description indeed. I must say I first tried to make the changes minimal but I can undertake a larger changes if it is generallyvthough to be useful.

Doc: clarify the meaning of the oparg for CACHE and COPY opcode in di…

ced36e4

…s doc

bedevere-bot added docs Documentation in the Doc dir skip news awaiting review labels Aug 31, 2022

kumaraditya303 requested a review from iritkatriel Aug 31, 2022

iritkatriel reviewed Aug 31, 2022

View changes

Doc/library/dis.rst Show resolved Hide resolved

Doc: move TOS, TOS1, etc definitions to the beginning of instruction …

23531e6

…description

iritkatriel requested a review from markshannon Sep 1, 2022

iritkatriel reviewed Sep 1, 2022

View changes

Doc: define TOS(i) and rewrite CACHE and SWAP description to use it

931b77c

iritkatriel reviewed Sep 1, 2022

View changes

typo

637f424

markshannon reviewed Oct 3, 2022

View changes

Doc.dis: remove use of TOSi and only use TOS(i)

aa0ee39

gh-96461: clarify the meaning of the oparg for CACHE and COPY opcode #96462

gh-96461: clarify the meaning of the oparg for CACHE and COPY opcode #96462

MatthieuDartiailh commented Aug 31, 2022 •

edited by bedevere-bot

iritkatriel Sep 1, 2022

MatthieuDartiailh Sep 1, 2022

iritkatriel Sep 1, 2022 •

edited

iritkatriel Sep 1, 2022

MatthieuDartiailh Sep 1, 2022

markshannon Oct 3, 2022

markshannon Oct 3, 2022

markshannon Oct 3, 2022

MatthieuDartiailh commented Oct 5, 2022

iritkatriel commented Oct 5, 2022

MatthieuDartiailh commented Oct 5, 2022

MatthieuDartiailh commented Oct 6, 2022

sweeneyde commented Oct 7, 2022

MatthieuDartiailh commented Oct 7, 2022

		@@ -382,6 +382,10 @@ The Python compiler currently generates the following bytecode instructions.

		General instructions

		In the following, TOS(i) refer to the i-th item on the stack which is index from 1.

	In the following, TOS(i) refer to the i-th item on the stack which is index from 1.
	In the following, TOS(i) refers to the i-th item on the stack which is indexed from 1.

gh-96461: clarify the meaning of the oparg for CACHE and COPY opcode #96462

Are you sure you want to change the base?

gh-96461: clarify the meaning of the oparg for CACHE and COPY opcode #96462

Conversation

MatthieuDartiailh commented Aug 31, 2022 • edited by bedevere-bot

iritkatriel Sep 1, 2022

Choose a reason for hiding this comment

MatthieuDartiailh Sep 1, 2022

Choose a reason for hiding this comment

iritkatriel Sep 1, 2022 • edited

Choose a reason for hiding this comment

iritkatriel Sep 1, 2022

Choose a reason for hiding this comment

MatthieuDartiailh Sep 1, 2022

Choose a reason for hiding this comment

markshannon Oct 3, 2022

Choose a reason for hiding this comment

markshannon Oct 3, 2022

Choose a reason for hiding this comment

markshannon Oct 3, 2022

Choose a reason for hiding this comment

MatthieuDartiailh commented Oct 5, 2022

iritkatriel commented Oct 5, 2022

MatthieuDartiailh commented Oct 5, 2022

MatthieuDartiailh commented Oct 6, 2022

sweeneyde commented Oct 7, 2022

MatthieuDartiailh commented Oct 7, 2022

MatthieuDartiailh commented Aug 31, 2022 •

edited by bedevere-bot

iritkatriel Sep 1, 2022 •

edited