-
Notifications
You must be signed in to change notification settings - Fork 456
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
Implement tableswitch and flexible control flow #153
Conversation
@@ -134,12 +134,12 @@ expr: | |||
( block <var> <expr>+ ) ;; = (label <var> (block <expr>+)) | |||
( if <expr> <expr> <expr> ) | |||
( if <expr> <expr> ) ;; = (if <expr> <expr> (nop)) | |||
( if_break <expr> <var> ) ;; = (if <expr> (break <var>) (nop)) |
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.
The proposed name in the design repo is br_if
; if you don't like it, let's discuss this in the design repo, instead of having you just arbitrarily edit things according to your fancy.
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.
Relax, I was about to do that. But then saw your latest reply. ;)
Updated to match WebAssembly/design#427 more closely. |
val if_ : expr * expr * expr option -> expr' | ||
val if_else : expr * expr * expr -> expr' | ||
val if_ : expr * expr -> expr' | ||
val if_branch : expr * var -> 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.
For direct correspondence with AstSemantics.md, could you rename to br_if
?
Generally lgtm with nits addressed. The ast.ml doesn't match what we'd probably want in the binary encoding (a table of indices), but that seems fine since that's already the case for many other nodes (we'll have to define "concrete" AST nodes when we get to binary encoding For Real) and the behavior seems to be equivalent to the table. |
Nits addressed, plus a few other internal naming inconsistencies. |
Thanks! I think README.md also needs updating of names. I'd wait to here what @sunfishcode thinks before merging though :) |
Closing; superseded by #163. |
Instead of RangeError. Fixes WebAssembly#153.
WAVM/WAVM#232 [simd] Add more literal tests for float-point comparison ops Part of WAVM/WAVM#195
It looks people have generally agreed on removing `unwind` in the MVP spec, as it overlaps with `catch_all` in functionality. Closes WebAssembly#153.
Bikeshedding:- Currently still namedswitch
, but we can rename it to be super-explicit if preferred.- For consistency, new jumps are namedif_break
,case_break
,default_break
, but we can change that to*_branch
or something if we decide to rename the existingbreak
operator as well.