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

Change the argument to grow memory to be in units of pages. #598

Merged
merged 1 commit into from Mar 28, 2016
Merged

Change the argument to grow memory to be in units of pages. #598

merged 1 commit into from Mar 28, 2016

Conversation

ghost
Copy link

@ghost ghost commented Mar 10, 2016

Some support for this change #594

The result is still in units of bytes?

@rossberg
Copy link
Member

lgtm

* `grow_memory` : grow linear memory by a given unsigned delta which
must be a multiple of the page size. Return the previous memory size.
* `grow_memory` : grow linear memory by a given unsigned delta of pages.
Return the previous memory size in bytes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, for consistency the result should also be in pages now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, and what about memory_size?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be consensus to change the argument to pages so perhaps that could land first and then you could champion a change of the memory_size to pages too in a follow up PR, or do people want it changed here too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, sgtm

@kripken
Copy link
Member

kripken commented Mar 25, 2016

As discussed here the spec and the design repos are out of sync until this lands. Any remaining issues here? Sounds like everything should be units of page size?

@lukewagner
Copy link
Member

lgtm (assuming we change return value of grow_memory in a follow-up PR). Merging based on other lgtm and general consensus to switch everything over to units of pages.

@lukewagner lukewagner merged commit 7af4b47 into WebAssembly:master Mar 28, 2016
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.

3 participants