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.

Sign up for GitHub

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
Loading

closes: gh-92932

@corona10 corona10 requested a review from colesbury May 19, 2022 08:46
@corona10 corona10 marked this pull request as ready for review May 19, 2022 08:46
@corona10 corona10 added the needs backport to 3.11 only security fixes label May 19, 2022
@corona10 corona10 force-pushed the gh-92932 branch 2 times, most recently from 3bf6f02 to 30c7a4b Compare May 19, 2022 08:55
@Fidget-Spinner
Copy link
Member

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

@corona10
Copy link
Member Author

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 commented May 19, 2022 •
edited
Loading 

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 reviewed changes
Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

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

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

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 09:02
@corona10
Copy link
Member Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@corona10
Copy link
Member Author

@brandtbucher gentle ping :)

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

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

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

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
@miss-islington
Copy link
Contributor

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 pull request Jun 3, 2022
@corona10 @miss-islington
pythongh-92932: dis._unpack_opargs should handle EXTENDED_ARG_QUICK (p…
1fc77e3 
…ythongh-92945)

(cherry picked from commit b013804)

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

corona10 commented Jun 3, 2022 •
edited
Loading

Thank you Brandt for the review!

@corona10 corona10 deleted the gh-92932 branch June 3, 2022 02:30
miss-islington added a commit that referenced this pull request 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 brandtbucher approved these changes

@colesbury colesbury Awaiting requested review from colesbury

@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner

Assignees

@brandtbucher brandtbucher

Labels
needs backport to 3.11 only security fixes
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