Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(5)

#26146: PEP 511: Add ast.Constant to allow AST optimizer to emit constants

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 10 months ago by victor.stinner
Modified:
1 year, 10 months ago
Reviewers:
storchaka, joe
CC:
brett.cannon, haypo, Benjamin Peterson, devnull_psf.upfronthosting.co.za, storchaka
Visibility:
Public.

Patch Set 1 #

Total comments: 12

Patch Set 2 #

Total comments: 6

Patch Set 3 #

Total comments: 12
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Include/Python-ast.h View 1 2 3 chunks +10 lines, -3 lines 2 comments Download
Include/asdl.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
Lib/ast.py View 1 2 4 chunks +30 lines, -22 lines 0 comments Download
Lib/test/test_ast.py View 1 2 1 chunk +53 lines, -0 lines 2 comments Download
Parser/Python.asdl View 1 2 2 chunks +6 lines, -1 line 0 comments Download
Parser/asdl.py View 1 2 1 chunk +2 lines, -1 line 0 comments Download
Parser/asdl_c.py View 1 2 2 chunks +21 lines, -0 lines 2 comments Download
Python/Python-ast.c View 1 2 8 chunks +79 lines, -0 lines 0 comments Download
Python/ast.c View 1 2 2 chunks +50 lines, -0 lines 4 comments Download
Python/compile.c View 1 2 5 chunks +19 lines, -5 lines 0 comments Download
Python/future.c View 1 2 1 chunk +4 lines, -1 line 0 comments Download
Python/symtable.c View 1 2 1 chunk +1 line, -0 lines 0 comments Download
Tools/parser/unparse.py View 1 2 4 chunks +27 lines, -2 lines 2 comments Download

Messages

Total messages: 6
storchaka_gmail.com
http://bugs.python.org/review/26146/diff/16376/Lib/ast.py File Lib/ast.py (right): http://bugs.python.org/review/26146/diff/16376/Lib/ast.py#newcode209 Lib/ast.py:209: elif isinstance(node, Constant): Can docstring be float? http://bugs.python.org/review/26146/diff/16376/Python/Python-ast.c File ...
1 year, 10 months ago #1
haypo
http://bugs.python.org/review/26146/diff/16376/Lib/ast.py File Lib/ast.py (right): http://bugs.python.org/review/26146/diff/16376/Lib/ast.py#newcode209 Lib/ast.py:209: elif isinstance(node, Constant): On 2016/01/18 22:48:14, storchaka wrote: > ...
1 year, 10 months ago #2
haypo
https://bugs.python.org/review/26146/diff/16405/Lib/test/test_ast.py File Lib/test/test_ast.py (right): https://bugs.python.org/review/26146/diff/16405/Lib/test/test_ast.py#newcode968 Lib/test/test_ast.py:968: nested_tuple, nested_frozenset ) useless space https://bugs.python.org/review/26146/diff/16405/Lib/test/test_ast.py#newcode971 Lib/test/test_ast.py:971: value2 = ...
1 year, 10 months ago #3
haypo
http://bugs.python.org/review/26146/diff/16405/Lib/test/test_ast.py File Lib/test/test_ast.py (right): http://bugs.python.org/review/26146/diff/16405/Lib/test/test_ast.py#newcode968 Lib/test/test_ast.py:968: nested_tuple, nested_frozenset ) On 2016/01/23 13:04:05, haypo wrote: > ...
1 year, 10 months ago #4
llllllllll
http://bugs.python.org/review/26146/diff/16418/Include/Python-ast.h File Include/Python-ast.h (right): http://bugs.python.org/review/26146/diff/16418/Include/Python-ast.h#newcode321 Include/Python-ast.h:321: I see whitespace here on the patch locally http://bugs.python.org/review/26146/diff/16418/Lib/test/test_ast.py ...
1 year, 10 months ago #5
haypo
1 year, 10 months ago #6
https://bugs.python.org/review/26146/diff/16418/Include/Python-ast.h
File Include/Python-ast.h (right):

https://bugs.python.org/review/26146/diff/16418/Include/Python-ast.h#newcode321
Include/Python-ast.h:321: 
On 2016/01/26 18:46:28, llllllllll wrote:
> I see whitespace here on the patch locally

This file is generated, I don't really control how indentation is generated (I
don't really want to spend time on it :-))

https://bugs.python.org/review/26146/diff/16418/Lib/test/test_ast.py
File Lib/test/test_ast.py (right):

https://bugs.python.org/review/26146/diff/16418/Lib/test/test_ast.py#newcode947
Lib/test/test_ast.py:947: code = compile(tree, "<string>", "exec")
On 2016/01/26 18:46:28, llllllllll wrote:
> should you also test compiling with eval and single?

Hum, I'm not sure that it's really useful for the unit test. It changes the
container, but not the content.

Well, I tried to enhance the unit test to test other modes, but ast.parse()
doesn't work well with compile() :-/

compile(ast.parse("1+1", "eval"), "test", "eval") raises TypeError: expected
Expression node, got Module.

https://bugs.python.org/review/26146/diff/16418/Parser/asdl_c.py
File Parser/asdl_c.py (right):

https://bugs.python.org/review/26146/diff/16418/Parser/asdl_c.py#newcode877
Parser/asdl_c.py:877: if (obj == Py_None || obj == Py_True || obj == Py_False) {
On 2016/01/26 18:46:28, llllllllll wrote:
> Is this optimization worth it? This code would be more clear if we just add
all
> the constants to the arena. Do you have any idea how much faster this is? If
we
> intend to keep it, should we also check for Py_Ellipsis?

Oh, my intent was not to optimize the code but to write a working function. My
code is based on obj2ast_object() which has a special case for Py_None.

I remove the code "if (obj == Py_None ...) { ... return 0; }" and test_ast still
works, so I will just push this change.

https://bugs.python.org/review/26146/diff/16418/Python/ast.c
File Python/ast.c (right):

https://bugs.python.org/review/26146/diff/16418/Python/ast.c#newcode135
Python/ast.c:135: validate_constant(PyObject *value)
On 2016/01/26 18:46:28, llllllllll wrote:
> Why do we need to check that it is one of these primitive types.

It matters in Python/compile.c. A code object must be serializable by the
marshal module, and this module only supports very few types.

> If the point of
> this is for optimizations to be made then shouldn't users be able to create
> constants of any kind?

ast.Constant is helpful to implement simple optimizations like constant folding
and constant propagation.

Please see the issue. I explained why I didn't add a second ast.Literal type:
https://bugs.python.org/issue26146#msg258621

Maybe later I will find how to handle ast.Literal correctly, but right now I
prefer to keep it simple (KISS!). In my optimizer, I'm only using ast.Constant,
I'm not using an ast.Literal type.

The optimizer *does* copy mutable types to "constants". It's not done by the
compiler, but at runtime:
https://fatoptimizer.readthedocs.org/en/latest/optimizations.html#copy-builti...

I'm using https://fatoptimizer.readthedocs.org/en/latest/fat.html#replace_consts
helper to "replace" constants at runtime.

https://bugs.python.org/review/26146/diff/16418/Python/ast.c#newcode289
Python/ast.c:289: PyErr_SetString(PyExc_TypeError, "invalid type in Constant");
On 2016/01/26 18:46:28, llllllllll wrote:
> I commented above that I would prefer to remove this validation. If we do want
> to keep it we should print what the type we recieved was and what the types
> allowed are. The allowed types could even just be a reference like, "see
> ast.allowed_constant_types" or something. If you have a tree with more than 1
or
> 2 constant nodes this will make debugging much easier.

Ok to add the invalid type in the error message.

Not ok to give a list of supported types, it would be too annoying to maintain
it.

I don't think that it's worth to add ast.allowed_constant_types, I have no use
case for such list.

https://bugs.python.org/review/26146/diff/16418/Tools/parser/unparse.py
File Tools/parser/unparse.py (right):

https://bugs.python.org/review/26146/diff/16418/Tools/parser/unparse.py#newco...
Tools/parser/unparse.py:470: elt = t.elts[0]
On 2016/01/26 18:46:28, llllllllll wrote:
> Any reason for this change? Seems unrelated

It is unrelated :-) I'm just too laze to push a change just for this minor
change.

IMHO the code is more readable like that.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7