-
-
Notifications
You must be signed in to change notification settings - Fork 32k
bpo-41463: Generate information about jumps from 'opcode.py' rather than duplicating it in 'compile.c' #21714
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
Conversation
…te it in 'compile.c'
Python/compile.c
Outdated
unsigned char i_opcode; | ||
int i_oparg; | ||
struct basicblock_ *i_target; /* target block (if jump instruction) */ | ||
int i_lineno; | ||
}; | ||
|
||
static int |
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.
static int | |
static inline int |
Python/compile.c
Outdated
return (_PyOpcode_RelativeJump[opcode>>5]>>(opcode&31))&1; | ||
} | ||
|
||
static int |
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.
static int | |
static inline int |
Python/compile.c
Outdated
is_relative_jump(struct instr *i) | ||
{ | ||
int opcode = i->i_opcode; | ||
return (_PyOpcode_RelativeJump[opcode>>5]>>(opcode&31))&1; |
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.
Can you define 5 and 31 as globals with a name (using #define or with static variables). That way this will read a bit better
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.
Look great, I left some minor comments
@@ -6122,7 +6125,7 @@ optimize_basic_block(basicblock *bb, PyObject *consts) | |||
struct instr *inst = &bb->b_instr[i]; | |||
int oparg = inst->i_oparg; | |||
int nextop = i+1 < bb->b_iused ? bb->b_instr[i+1].i_opcode : 0; | |||
if (inst->i_jabs || inst->i_jrel) { | |||
if (is_jump(inst)) { |
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.
Great!
Python/compile.c
Outdated
static inline int | ||
is_relative_jump(struct instr *i) | ||
{ | ||
int opcode = i->i_opcode; |
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.
Apologies for insisting a bit but it still took me a minute to parse the expression to follow how the function is determining if is a relative jump. I would suggest to maybe add a comment here briefly explaining the process as someone with less context will certainly struggle
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
I left a comment, feel free to address it before landing
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.
Perfect!
…han duplicating it in 'compile.c' (pythonGH-21714) Generate information about jumps from 'opcode.py' rather than duplicate it in 'compile.c'
…han duplicating it in 'compile.c' (pythonGH-21714) Generate information about jumps from 'opcode.py' rather than duplicate it in 'compile.c'
The motivation for this PR is to avoid errors when modifying compile.c.
Setting the
i_jabs
andi_jrel
fields of every instruction is error prone and unnecessary.https://bugs.python.org/issue41463