-
Notifications
You must be signed in to change notification settings - Fork 197
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
Lowering pass and augmented assignments #338
Conversation
6801c55
to
aead595
Compare
Codecov Report
@@ Coverage Diff @@
## master #338 +/- ##
==========================================
+ Coverage 92.60% 92.77% +0.16%
==========================================
Files 56 59 +3
Lines 3882 4000 +118
==========================================
+ Hits 3595 3711 +116
- Misses 287 289 +2
Continue to review full report at Codecov.
|
548d5f9
to
fb0f056
Compare
unreachable!() | ||
} | ||
|
||
fn func_stmt(context: &Context, stmt: Node<fe::FuncStmt>) -> Vec<Node<fe::FuncStmt>> { |
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.
Note: returning a vec here because we'll have lowerings like:
(foo, bar): (u256, u256) = my_tuple
to
foo: u256 = my_tuple.item0
bar: u256 = my_tuple.item1
which requires that we be able to map a single statement to multiple statements.
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.
This is a good candidate for SmallVec<[Node<..>, 1]>
in some possible future performance optimization.
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.
Makes sense. I'll track this in an issue.
fb0f056
to
815e4bf
Compare
@@ -278,7 +278,7 @@ pub fn expr_bin_operation(context: &Context, exp: &Node<fe::Expr>) -> yul::Expre | |||
} | |||
_ => unreachable!(), | |||
}, | |||
_ => unimplemented!(), | |||
fe::BinOperator::FloorDiv => unimplemented!(), |
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.
see #341
@@ -0,0 +1,57 @@ | |||
contract Foo: |
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 would be nice to have some tests that check that source files are correctly lowered. We need to be able to pretty-print ASTs for this, tho. See #326.
815e4bf
to
4f4f5a8
Compare
value, | ||
} = &stmt.kind | ||
{ | ||
let target_attributes = expressions::expr(Rc::clone(&scope), Rc::clone(&context), target)?; |
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.
relevant: #345
|
||
/// Lowers a boxed expression. | ||
#[allow(clippy::boxed_local)] | ||
pub fn boxed_expr(context: &Context, exp: Box<Node<fe::Expr>>) -> Box<Node<fe::Expr>> { |
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.
This is another pattern that might be worth optimizing in the future, or might not matter; we take a boxed expr, probably don't modify it, and rebox. An extra allocation for every expr.
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. My only (mild) concern is potential performance issues with all the allocations happening. Probably won't matter in practice, and if it does we can work on that later. (See inline comments above).
Added a lowering pass to the compiler module and used it to support augmented assignments.
e.g.
foo += 1
becomesfoo = foo + 1
To-Do
OPTIONAL: Update Spec if applicable
Add entry to the release notes (may forgo for trivial changes)
Clean up commit history