-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat!: Update libbinaryen to v105 #142
Conversation
f18d443
to
8807ea4
Compare
23d88f2
to
ae7a71d
Compare
@ospencer this is ready for review! |
@@ -98,6 +98,7 @@ caml_binaryen_element_segment_get_name(value _elem) { | |||
CAMLreturn(caml_copy_string(BinaryenElementSegmentGetName(elem))); | |||
} | |||
|
|||
// TODO: Expose to bindings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we do this now or no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't do it now because the JS version doesn't actually expose this at all! I was so happy to find that we didn't implement it 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! When I initially looked this over, all of the implementations looked reasonable/correct, though of course the eyes tend to start glazing over after a certain point. I didn't see any obvious mistakes, so let's ship it! Do you think it's possible to run the Grain test suite in JS? It sounds possible but feels difficult 😛
I can give it a try with this new JSOO structure! But let's get this merged first. I'll try it before releasing the new version. |
Closes #104
Once completed, this will update the underlying libbinaryen to v105.1.0 and switch out the virtual module for more standard js_of_ocaml externals.
This was a beast of an undertaking, so the review will probably take a while.