-
Notifications
You must be signed in to change notification settings - Fork 454
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
initial tests for globals #57
Conversation
Thanks! I think you also need to add a rule to check.ml to allow expressions like assigning a |
regarding removing globals, no problem, I'm using this as a training to understand the project and the spec so no problem for me throwing this later :). Just out of curiosity, will globals be removed for good or there are plans to reintroduce them later? I think globals are easily "shimed" with module memory so maybe they are not that primitive... |
@marianoguerra Globals would just be removed from the MVP. See the new #344 (which, when merged, will be followed by a PR in this repo) for more explanation. |
@@ -179,14 +179,14 @@ let rec check_expr c ts e = | |||
|
|||
| SetLocal (x, e1) -> | |||
check_expr c [local c x] e1; | |||
check_type [] ts e.at | |||
check_type [local c x] ts e.at |
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.
Here and below, it might be nice to factor out via let t1 = local c x in
to avoid duplicating the expression.
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.
I will, just need to read an introduction to ocaml somewhere, any good recommendation? (I already code in clojure, erlang and scheme so it can be a high level introduction)
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.
FWIW, I actually find it more readable (since shorter) as is. :)
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.
Hehe, fair enough. I was on the edge anyhow for such a simple duplicated expr.
Should we avoid merging this, based on WebAssembly/design#344 removing globals from MVP? |
@jfbastien I can remove globals and merge the other changes if it's ok for you. |
Sounds good. Though this needs a new description and name :) |
If you update this PR to remove globals, I think we can merge it. |
Created #106 which subsumes this PR. |
Merge remote-tracking branch spec/,master
this are some simple tests to check that globals are set and return the set value both from inside the module and from outside and that setting two different globals doesn't affect each other.
this is not in sync with the spec which says that store should return the stored value, will look into it now.