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

binaryen.ml.js #15

Merged
merged 3 commits into from
Jul 22, 2020
Merged

binaryen.ml.js #15

merged 3 commits into from
Jul 22, 2020

Conversation

phated
Copy link
Member

@phated phated commented Jul 21, 2020

This builds on top of #14 and adds OCaml bindings to the Binaryen.js library (I even submoduled it, whoa!)

There are a couple of TODOs around checking bytes/Uint8Arrays but we need to expand the test suite or use it in Grain to try those out.

@phated phated requested a review from ospencer July 21, 2020 07:01

let dispose wasm_mod = ignore (meth_call wasm_mod "dispose" [||])

(* TODO: Check the unit8Array conversion *)
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't do ANY checks for this, like I did for the module writing/reading.

@@ -0,0 +1,2 @@
// Attach binaryen to our global
global.binaryen = module.exports;
Copy link
Member Author

Choose a reason for hiding this comment

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

Lulz 😭

@@ -11,3 +11,16 @@ external float64_bits : int64 -> t = "caml_binaryen_literal_float64_bits"
let float32 n = float32_bits @@ Int32.bits_of_float n

external float64 : float -> t = "caml_binaryen_literal_float64"

(* Hacks for Binaryen.js stack allocations, Don't use in binaryen.native *)
type jsoo =
Copy link
Member Author

Choose a reason for hiding this comment

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

This is absolutely terrible, but I couldn't find a way around it while still using Binaryen.js because their const implementations use their custom stack.

test/test_js.expected Outdated Show resolved Hide resolved
Comment on lines +91 to +96
let uint8array_to_bytes u8a =
Bytes.of_string (Typed_array.String.of_uint8Array u8a)

let bytes_to_uint8array byts =
meth_call Typed_array.uint8Array "from"
[| inject (string (Bytes.to_string byts)) |]
Copy link
Member Author

Choose a reason for hiding this comment

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

These are the byte/uint8array utils that need to be checked.

Copy link
Member

@ospencer ospencer left a comment

Choose a reason for hiding this comment

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

🤯 This is truly insane. Well done! I'm pretty ready to start putting it through the paces 👏

Only comment I really think I have: would it make sense to put js/binaryen.js in src/binaryen.js to match the other source submodule?

test/test_js.expected Outdated Show resolved Hide resolved
@phated
Copy link
Member Author

phated commented Jul 21, 2020

@ospencer I think keeping the binaryen.js submodule in the JS directory is better for now, I didn't really want to deal with dune needing to reach into directories up the tree to get at the binaryen.js/index.js source file.

If we ever get the main binaryen to build into JS off that main submodule, we might be able to drop the binaryen.js submodule completely.

@phated phated merged commit b0e19b0 into master Jul 22, 2020
@phated phated deleted the binaryen-js branch July 22, 2020 02:26
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.

None yet

2 participants