bpo-29469: Move constant folding at AST level by methane · Pull Request #2858 · python/cpython
bpo-29469: Move constant folding at AST level
bpo-29469: Move constant folding at AST level (wip, don't review)
bpo-29469: Move constant folding at AST level (wip, don't review)
bpo-29469: Move constant folding at AST level
Overall, this looks great. The code is remarkably clean.
Python/importlib.h is conflicted very often. So I'll fix it right before merge.
Could you please remove asdl_ct.py and simplify ast_opt.c by getting rid of useless functions like astfold_boolop? The AST types are rarely changed, and I think it would be not hard to update the cleaner code. If sometimes we will need the general code generator we could restore it from your patches.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be worth to document this change in What's New. Even without additional optimizations it can optimize more than the peephole optimizer.
| case NameConstant_kind: | ||
| return e->v.NameConstant.value; | ||
| default: | ||
| assert(!is_const(e)); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Py_UNREACHABLE() is now used in compile.c.
| } | ||
| } | ||
|
|
||
| static int make_const(expr_ty node, PyObject *val, PyArena *arena) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move static int to a separate line. And similarly in other functions below.
|
|
||
| typedef PyObject *(*unary_op)(PyObject*); | ||
| static const unary_op ops[] = { | ||
| 0, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK in C99 you can write
[Invert] = PyNumber_Invert, [Not] = unary_not, ...
| Py_ssize_t size = PyObject_Size(newval); | ||
| if (size == -1) { | ||
| if (PyErr_ExceptionMatches(PyExc_KeyboardInterrupt)) | ||
| return 1; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newval leaked.
| } | ||
|
|
||
| /* Avoid creating large constants. */ | ||
| Py_ssize_t size = PyObject_Size(newval); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newval can be NULL.
| static int make_const(expr_ty node, PyObject *val, PyArena *arena) | ||
| { | ||
| if (val == NULL) { | ||
| if(!PyErr_ExceptionMatches(PyExc_KeyboardInterrupt)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed space after if. Missed braces.
| !is_const(arg) || | ||
| /* TODO: handle other types of slices */ | ||
| slice->kind != Index_kind || | ||
| !is_const(slice->v.Index.value)) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move { to a new line.
|
|
||
| #undef CALL | ||
| #undef CALL_OPT | ||
| #undef CALL_SEQ |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #include "Python-ast.h" | ||
|
|
||
|
|
||
| /* TODO: is_const and get_const_value is copied from Python/compile.c. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/is/are/ ?
| 'second': [1, 2], | ||
| 'third': [3, 4], | ||
| }) | ||
| self.assertIsNot(pl2['first'], pl2['second']) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! This test can fail in other implementations. It should be fixed in 3.6 too. Opened #4813 for this.
| if (size == -1) { | ||
| if (PyErr_ExceptionMatches(PyExc_KeyboardInterrupt)) | ||
| return 1; | ||
| Py_DECREF(newval); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only before return.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if fix a one simple error.
Generated files not up to date.
Great! Many thanks @methane for your work!
Now I'll try to implement additional optimizations.
| expr_ty arg = node->v.UnaryOp.operand; | ||
|
|
||
| if (!is_const(arg)) { | ||
| /* Fold not into comparison */ |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this makes this peephole pass unnecessary too?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're right.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #4863. Thanks.