cpython
cpython copied to clipboard
python
gh-96461: clarify the meaning of the oparg for CACHE and COPY opcode
This could be backported to Python 3.11
- Issue: gh-96461
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 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?
Removing the TOSi should be a smaller change yes and would not entail any large duplication.
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.
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., dostack.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?
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.
I went ahead with @sweeneyde suggestion and also fixed a bunch of small issues in separate commits (let me know if they should be split into their own PR). I will wait to get some feedback before dealing with the merge conflict.
Thanks @iritkatriel. I will incorporate your changes and work on fixing the conflicts.
![cpython-cla-bot[bot] avatar](https://avatars.githubusercontent.com/in/171517?v=4 "Published by a pub.dev verified publisher"){kind=small}
I have no issue with the term TOS. I am simply looking for the best way to document opcode operating on the i-th element of the stack. Ideally I would like whatever is picked to be easy to remember and ideally consistent across the documentation. I like STACK[-i] because to me at least it makes it very obvious that the indexing starts at 1. But I would happily make whichever changes are believed to be best.
I rebased this and forced pushed.
There are places where using TOS instead of STACK[-1] may be more readable and make this change less disruptive but it would hurt uniformity. @iritkatriel @gvanrossum let me know which version you prefer and I will make the relevant changes
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.
Thanks @markshannon I addressed the points you raised and some other similar issues.
Thanks for making the requested changes!
@markshannon: please review the changes made to this pull request.
I rebased to address the conflicts and updated the description of CALL_INSTRINSIC_1
ping @markshannon
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.
I updated BUILD_TUPLE based on my response to your comment which I believe is correct. Correct if I somehow got it wrong still.
I have made the requested changes; please review again
Thanks for making the requested changes!
@markshannon: please review the changes made to this pull request.
