Skip to content
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

Closed
wants to merge 6 commits into from

Conversation

marianoguerra
Copy link

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.

@lukewagner
Copy link
Member

Thanks! I think you also need to add a rule to check.ml to allow expressions like assigning a set_global to a local to succeed. Just a fair warning, we're seriously considering removing globals from the MVP (and so ml-proto) in #154. However the analogous change you've made here also applies to set_local and store.

@marianoguerra
Copy link
Author

  • added the change in check for store_global
  • added test to check that I can use the result of store_global
  • added the ability to return the value being set on set_local
  • added test to check that I can use the result of set_local

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...

@lukewagner
Copy link
Member

@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
Copy link
Member

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.

Copy link
Author

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)

Copy link
Member

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. :)

Copy link
Member

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.

@jfbastien
Copy link
Member

Should we avoid merging this, based on WebAssembly/design#344 removing globals from MVP?

@marianoguerra
Copy link
Author

@jfbastien I can remove globals and merge the other changes if it's ok for you.

@jfbastien
Copy link
Member

Sounds good. Though this needs a new description and name :)

@lukewagner
Copy link
Member

If you update this PR to remove globals, I think we can merge it.

@lukewagner
Copy link
Member

Created #106 which subsumes this PR.

@lukewagner lukewagner closed this Oct 2, 2015
littledan pushed a commit to littledan/spec that referenced this pull request Mar 4, 2018
eqrion pushed a commit to eqrion/wasm-spec that referenced this pull request Jul 18, 2019
alexcrichton pushed a commit to alexcrichton/spec that referenced this pull request Nov 18, 2019
Merge remote-tracking branch spec/,master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants