X Tutup
The Wayback Machine - https://web.archive.org/web/20220603145343/https://github.com/python/cpython/pull/92945
Skip to content
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

Jump to bottom

gh-92932: dis._unpack_opargs should handle EXTENDED_ARG_QUICK #92945

Merged
merged 4 commits into from Jun 3, 2022
Merged

gh-92932: dis._unpack_opargs should handle EXTENDED_ARG_QUICK #92945

merged 4 commits into from Jun 3, 2022

Conversation

corona10
Copy link
Member

@corona10 corona10 commented May 19, 2022 •
edited

closes: gh-92932

@corona10 corona10 requested a review from colesbury May 19, 2022
@corona10 corona10 marked this pull request as ready for review May 19, 2022
@corona10 corona10 requested a review from Fidget-Spinner May 19, 2022
@corona10 corona10 force-pushed the gh-92932 branch 2 times, most recently from 3bf6f02 to 30c7a4b Compare May 19, 2022
@Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented May 19, 2022

Sorry Dong-hee, I'm useless at dis. Can't help you review this here :).

@corona10 corona10 requested a review from brandtbucher May 19, 2022
@corona10
Copy link
Member Author

@corona10 corona10 commented May 19, 2022

Sorry Dong-hee, I'm useless at dis. Can't help you review this here :).

Okay, Then I need to request this review from Brandt.

@corona10
Copy link
Member Author

@corona10 corona10 commented May 19, 2022 •
edited 

230261,230262c230261,230262
<            1118398 EXTENDED_ARG_QUICK       1
<            1118400 LOAD_GLOBAL            248 (g125)
---
>            1118398 EXTENDED_ARG_QUICK     513
>            1118400 LOAD_GLOBAL          131576 (g65789)
230265,230266c230265,230266
<            1118416 EXTENDED_ARG_QUICK       1
<            1118418 LOAD_GLOBAL            250 (g126)
---
>            1118416 EXTENDED_ARG_QUICK     513
>            1118418 LOAD_GLOBAL          131578 (g65790)
230269,230270c230269,230270
<            1118434 EXTENDED_ARG_QUICK       1
<            1118436 LOAD_GLOBAL            252 (g127)
---
>            1118434 EXTENDED_ARG_QUICK     513
>            1118436 LOAD_GLOBAL          131580 (g65791)
230273,230274c230273,230274
<            1118452 EXTENDED_ARG_QUICK       1
<            1118454 LOAD_GLOBAL            254 (g128)
---
>            1118452 EXTENDED_ARG_QUICK     513
>            1118454 LOAD_GLOBAL          131582 (g65792)
230277,230278c230277,230278
<            1118470 EXTENDED_ARG_QUICK       2
<            1118472 LOAD_GLOBAL              0 (g1)
---
>            1118470 EXTENDED_ARG_QUICK     514
>            1118472 LOAD_GLOBAL          131584 (g65793)
230281,230282c230281,230282
<            1118488 EXTENDED_ARG_QUICK       2
<            1118490 LOAD_GLOBAL              2 (g2)
---
>            1118488 EXTENDED_ARG_QUICK     514
>            1118490 LOAD_GLOBAL          131586 (g65794)

brandtbucher
brandtbucher requested changes May 19, 2022
View changes
Copy link
Member

@brandtbucher brandtbucher left a comment

Thanks for the fix!

This should also have a test in test_dis. As a simple example, the statement *_, _ = ... should always generate an instruction UNPACK_EX(256) (proving that EXTENDED_ARG works correctly).

Lib/dis.py Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented May 19, 2022

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@corona10 corona10 force-pushed the gh-92932 branch 4 times, most recently from f392232 to bd236e0 Compare May 20, 2022
@corona10
Copy link
Member Author

@corona10 corona10 commented May 20, 2022

I have made the requested changes; please review again

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented May 20, 2022

Thanks for making the requested changes!

@brandtbucher: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from brandtbucher May 20, 2022
@corona10
Copy link
Member Author

@corona10 corona10 commented May 23, 2022

@brandtbucher gentle ping :)

brandtbucher
brandtbucher approved these changes Jun 2, 2022
View changes
Copy link
Member

@brandtbucher brandtbucher left a comment

Looks good! I found a couple of spelling nits, but feel free to merge otherwise:

Lib/test/test_dis.py Outdated Show resolved Hide resolved
Lib/test/test_dis.py Outdated Show resolved Hide resolved
Lib/test/test_dis.py Outdated Show resolved Hide resolved
@corona10 corona10 merged commit b013804 into python:main Jun 3, 2022
13 checks passed
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jun 3, 2022

Thanks @corona10 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jun 3, 2022
@corona10 @miss-islington
pythongh-92932: dis._unpack_opargs should handle EXTENDED_ARG_QUICK (p…
1fc77e3 
…ythongh-92945)

(cherry picked from commit b013804134b07894205b06744628f6b25b879d85)

Co-authored-by: Dong-hee Na <donghee.na@python.org>
@corona10
Copy link
Member Author

@corona10 corona10 commented Jun 3, 2022 •
edited

Thank you Brandt for the review!

@corona10 corona10 deleted the gh-92932 branch Jun 3, 2022
miss-islington added a commit that referenced this issue Jun 3, 2022
@miss-islington @corona10
gh-92932: dis._unpack_opargs should handle EXTENDED_ARG_QUICK (gh-92945)
e798f69 
(cherry picked from commit b013804)

Co-authored-by: Dong-hee Na <donghee.na@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brandtbucher brandtbucher

@colesbury colesbury

@Fidget-Spinner Fidget-Spinner

Assignees

@brandtbucher brandtbucher

Labels
needs backport to 3.11
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

dis._unpack_opargs should handle EXTENDED_ARG_QUICK
5 participants
@corona10 @Fidget-Spinner @bedevere-bot @miss-islington @brandtbucher
X Tutup