binaryen icon indicating copy to clipboard operation
binaryen copied to clipboard

feat(js): Extract *info functions into individual functions

Open phated opened this issue 2 years ago • 5 comments

This is a draft that builds upon #4750

Binaryen.ml provides separate functions for each of these utilities, but using the getMemorySegmentInfoByIndex helper function does a lot of extra work on each function call. By providing these under a namespace, the individual functions can be provided and then used in the "info" helper.

I'd like to do this to the other "info" helpers in the JS wrapper, but wanted to gather feedback first.

phated avatar Jun 24 '22 00:06 phated

Reminds me of the existing classes for Expression, Block, Function etc., which provide both namespaced static functions like Function.getName and convenient wrapper classes with getters for Function#name etc. through deriveWrapperInstanceMembers. Not all of the elements have a class yet, but perhaps here there could be a MemorySegment class (perhaps on top of a MemorySegmentRef) utilizing the existing concept?

MemorySegment
 .getOffset
 .setOffset
 .getData
 .setData
 .isPassive
 .setPassive
 #offset [get/set]
 #data [get/set]
 #passive [get/set]

Here's what's done for functions, for example:

https://github.com/WebAssembly/binaryen/blob/98016931d9de23c0b6a4b5b0df5d4c3954da2f39/src/js/binaryen.js-post.js#L4766-L4818

dcodeIO avatar Jun 24 '22 12:06 dcodeIO

Yeah, I was wondering if doing something similar to Expressions was desirable. I'm fine implementing like that, but it'll add more work, which I'll have to find some time to do.

Is there a way to move forward with the straightforward "instance logic" that I did or would the PR only be accepted if the static/class implementation were completed?

phated avatar Jun 24 '22 20:06 phated

cc @ashleynh, who has worked on the internal representation of data segments in support of multiple memory. @dcodeIO's idea to reuse that class utility sounds like a good goal to me, but intermediate progress might be useful to land as well.

tlively avatar Jun 28 '22 00:06 tlively

Thanks for the guidance folks! I tried to replicate patterns I had seen elsewhere in binaryen.js-post.js. Please take a look and let me know if I should be doing anything differently.

I've updated the getSomethingInfo functions that we want to remove in Grain but there are still more. Ideally, this PR can guide others in updating the rest (and of course I'll be updating things as we use them in Grain).

phated avatar Jul 08 '22 21:07 phated

@kripken thanks for the feedback! I've made the updates you suggested

phated avatar Jul 12 '22 17:07 phated