Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upbpo-33387: Simplify bytecodes for try-finally, try-except and with blocks. #6641
Conversation
For the record, here is the result of pyperformance.
|
0dfe44c
to
d8b54c9
@markshannon FYI, |
d8b54c9
to
3efb201
3efb201
to
b16b8e8
All comments addressed and rebased to fix merge conflict with ddd1949 |
b16b8e8
to
d64244b
Rebased again |
d64244b
to
6f916e9
Rebased yet again. |
b9c698e
to
cdb2581
Rebased yet again (twice) |
Mostly stylistic stuff and requests for comments to help explain the code in some spots. |
|
||
Calls the function in position 7 on the stack with the top three | ||
items on the stack as arguments. | ||
Used to implement the call ``exit(*exc_info())`` when an exception |
brettcannon
Jun 7, 2019
Member
This was initially misleading to me as my brain thought of sys.exit()
instead of context_manager.__exit__()
.
Used to implement the call ``exit(*exc_info())`` when an exception | |
Used to implement the call ``context_manager.__exit__(*exc_info())`` when an exception |
} codetracker; | ||
|
||
static void | ||
reset(codetracker *tracker) |
brettcannon
Jun 7, 2019
Member
Perhaps a comment as to why some fields don't require resetting while others do? And maybe why the fields in init_codetracker()
differ from those in reset()
? Basically my brain is wondering why e.g. start_line
is initialized but not reset, and vice-versa.
There's also a naming discrepancy between reset()
and init_codetracker()
in that one includes codetracker
and the other does not.
} | ||
while (tracker->offset < tracker->lnotab_len && | ||
tracker->addr >= tracker->line_addr + tracker->lnotab[tracker->offset]) { | ||
tracker->line_addr = tracker->line_addr + tracker->lnotab[tracker->offset]; |
brettcannon
Jun 7, 2019
Member
tracker->line_addr = tracker->line_addr + tracker->lnotab[tracker->offset]; | |
tracker->line_addr += tracker->lnotab[tracker->offset]; |
return op == SETUP_FINALLY && !is_try_except(op, target_op) && !is_async_for(op, target_op); | ||
} | ||
|
||
#define TRY_EXCEPT 250 |
brettcannon
Jun 7, 2019
Member
I would expect to find this value in opcode.h
but I'm not, so why the new definition of an opcode here? (A comment would be enough for me to clarify why this is here.)
} | ||
else if (is_try_finally(op, target_op)) { | ||
int addr = tracker->addr; | ||
// Skip over duplicate finally blocks if line is after body. |
brettcannon
Jun 7, 2019
Member
// Skip over duplicate finally blocks if line is after body. | |
// Skip over duplicate 'finally' blocks if line is after body. |
PyErr_SetString(PyExc_ValueError, | ||
"can't jump into the middle of a block"); | ||
return -1; | ||
/* Validate change of block stack */ |
brettcannon
Jun 7, 2019
Member
/* Validate change of block stack */ | |
/* Validate change of block stack. */ |
} | ||
break; | ||
} | ||
/* Check for illegal jumps out of finally or except blocks */ |
brettcannon
Jun 7, 2019
Member
/* Check for illegal jumps out of finally or except blocks */ | |
/* Check for illegal jumps out of finally or except blocks. */ |
@@ -2587,7 +2620,7 @@ compiler_for(struct compiler *c, stmt_ty s) | |||
if (start == NULL || end == NULL || cleanup == NULL) | |||
return 0; | |||
|
|||
if (!compiler_push_fblock(c, FOR_LOOP, start, end)) | |||
if (!compiler_push_fblock(c, FOR_LOOP, start, end, NULL)) |
brettcannon
Jun 7, 2019
Member
If you're up for it, would you mind fixing these if
blocks to use {}
? Otherwise the style is shifting and I would rather promote the new style than the old one.
compiler_use_next_block(c, finally); | ||
if (!compiler_push_fblock(c, FINALLY_END, finally, NULL)) | ||
|
||
/* End of body; start the cleanup */ |
brettcannon
Jun 7, 2019
Member
/* End of body; start the cleanup */ | |
/* End of body; start the cleanup. */ |
All review comments addressed. |
FYI no need to wait on me on an approving review. |
db08c6d
to
c611d1b
Rebased yet again... Time to press that merge button |
@markshannon is there anything preventing you from doing the merge at this point? |
c611d1b
to
1bd40b8
@brettcannon No technical obstacles, but I think that for large PRs like this one, it is best if someone other than the author does the merge. |
@markshannon Are you choosing to skip my doc change suggestions? (I'm asking because you said all the comments were addressed but those were left open/unresolved, so I just wanted to check the PR is in its final state sans merge conflict.) As for clicking "approved" on a review, that's as close as people typically get for approval on a core dev's PR. Historically core devs have done their own merges as they will be the ones to handle reversions, etc. IOW if you want to address my remaining doc comments and are willing to handle reversion duties, etc., then I can click the merge button for you. |
1bd40b8
to
385b2df
54f381c
to
23cfe89
23cfe89
to
62a3625
62a3625
to
8457a00
…parate code for normal and exceptional paths. Remove BEGIN_FINALLY, END_FINALLY, CALL_FINALLY and POP_FINALLY bytecodes. Implement finally blocks by code duplication. Reimplement frame.lineno setter using line numbers rather than bytecode offsets.
8457a00
to
6364ee7
I've been holding off on this as I wanted to let the 3.8 release stabilize before changing the bytecode. @brettcannon I have addressed all your comments. |
@markshannon: Please replace |
…parate code for normal and exceptional paths. (python#6641) Remove BEGIN_FINALLY, END_FINALLY, CALL_FINALLY and POP_FINALLY bytecodes. Implement finally blocks by code duplication. Reimplement frame.lineno setter using line numbers rather than bytecode offsets.
…parate code for normal and exceptional paths. (python#6641) Remove BEGIN_FINALLY, END_FINALLY, CALL_FINALLY and POP_FINALLY bytecodes. Implement finally blocks by code duplication. Reimplement frame.lineno setter using line numbers rather than bytecode offsets.
Quoting the bpo issue:
The implementation of
with
statements is a bit more complex. The with statementis implemented as:
https://bugs.python.org/issue33387