-
Notifications
You must be signed in to change notification settings - Fork 1
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
desugar more types of for-loops #10
Conversation
Pull Request Test Coverage Report for Build 264
💛 - Coveralls |
Actually, don't merge this yet. |
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.
Looks good to me in its current state, let me know when you think it's ready for another review/merge
silica/compile.py
Outdated
tree, list_lens = propagate_types(tree) | ||
tree, loopvars = desugar_for_loops(tree, list_lens) | ||
|
||
ast_utils.print_ast(tree) |
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.
Let's take this out before we merge. More generally it would be good to set up some logging infrastructure so we could have these print outs hidden under a DEBUG
level. I'll put that on the todo list.
return "<MemoryType: {}, {}>".format(self.height, self.width) | ||
|
||
def __repr__(self): | ||
return "<MemoryType: {}, {}>".format(self.height, self.width) |
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.
We could just have __str__
call __repr__
since right now they're the same. In fact, I think if you only define __repr__
, __str__
will default to calling __repr__
ast.Assign([ast.Name(index, ast.Store())], start), | ||
ast.While(ast.BinOp(ast.Name(index, ast.Load()), ast.Lt(), stop), | ||
[ast.Assign([ast.Name(node.target.id, ast.Store())], | ||
ast.Subscript(node.iter, ast.Index(ast.Name(index, ast.Load()))))] + |
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.
It looks like travis is unhappy about this Subscript
constructor call. Are you using Python 3.6? The AST has had some changes in Python 3, so that could explain the discrepancy if you're not seeing the issue locally. Subscript expects value, slice, ctx
(see https://greentreesnakes.readthedocs.io/en/latest/nodes.html#Subscript), so I think this is missing a ctx
. I think in this case it would be an ast.Load()
, since we're loading the subscripted value and storing into the loop var
veriloggen doesn't seem to support unpacked array syntax
for x in range(1,10,1)
for x in range(len(A))
for x in X