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

bpo-46528: Simplify BUILD_TUPLE/UNPACK_SEQUENCE folding #31039

Merged
merged 1 commit into from Feb 1, 2022

Conversation

@brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Jan 31, 2022

Also, fix test_pack_unpack.

https://bugs.python.org/issue46528

Copy link
Contributor

@markshannon markshannon left a comment

One nit about continue vs break. Otherwise LGTM.

case BUILD_TUPLE:
if (nextop == UNPACK_SEQUENCE && oparg == bb->b_instr[i+1].i_oparg) {
switch(oparg) {
case 1:
inst->i_opcode = NOP;
bb->b_instr[i+1].i_opcode = NOP;
break;
continue;
Copy link
Contributor

@markshannon markshannon Feb 1, 2022

Choose a reason for hiding this comment

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

Why change this? Aren't they equivalent.
Having a continue here distracts from the flow of the code, as it is not the normal way to end a case and increases cognitive load (at least it did for me).

Copy link
Member Author

@brandtbucher brandtbucher Feb 1, 2022

Choose a reason for hiding this comment

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

They’re not quite equivalent: note that I also removed the break on old line 8675.

Previously, values other than 1, 2, or 3 wouldn’t get a chance to hit fold_tuple_on_constants. Now they do.

Copy link
Contributor

@markshannon markshannon Feb 1, 2022

Choose a reason for hiding this comment

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

👍 LGTM then.

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Feb 1, 2022

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

And if you don't make the requested changes, you will be poked with soft cushions!

@brandtbucher brandtbucher merged commit a0e55a5 into python:main Feb 1, 2022
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants