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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM then.
When you're done making the requested changes, leave the comment: And if you don't make the requested changes, you will be poked with soft cushions! |
Also, fix
test_pack_unpack
.https://bugs.python.org/issue46528