binaryen

binaryen copied to clipboard

Maybe we should put an assertion here so we don't forget about this. _Originally posted by @tlively in https://github.com/WebAssembly/binaryen/pull/4811#discussion_r938221231_

ashleynh

These 5 lines seem to repeat a lot - maybe a helper function in this file? _Originally posted by @kripken in https://github.com/WebAssembly/binaryen/pull/4811#discussion_r933657076_

ashleynh

Worth a comment here that we assume all memories have the same type (all 32 or all 64). _Originally posted by @kripken in https://github.com/WebAssembly/binaryen/pull/4811#discussion_r933655909_

ashleynh

Does this return the memory index, or something else? Worth a comment here I think. _Originally posted by @kripken in https://github.com/WebAssembly/binaryen/pull/4811#discussion_r933621856_

ashleynh

This pass can probably halt with an error (like on line 49 above) if there are no memories. Please also add a comment that we are assuming that memory 0...

ashleynh

I see we have existing bugs here, but every `strToStack` call needs to be wrapped in a `preserveStack` call to avoid permanently leaking stack space. _Originally posted by @tlively in...

ashleynh

I wonder if we should produce a fatal error here (and in similar places) so we don't forget to come back and fix it up later. @kripken, wdyt? _Originally posted...

ashleynh

Since we are producing a fatal error anyway, might as well call `getMemory` instead of `getMemoryOrNull` and skip the error checking here. _Originally posted by @tlively in https://github.com/WebAssembly/binaryen/pull/4811#discussion_r933621253_

ashleynh

Refactor BinaryenSetMemory API

3

In a follow-up we'll want to refactor this API so that segments are not so closely tied to memories. Looking at the behavior below, it looks like this function blows...

ashleynh

I wonder if it's worth having a method like `wasm.getOnlyMemory` that asserts that there is exactly one memory and returns is. Then finding all the places that need to be...

ashleynh