BinaryenSetFunctionTable now accepts array of func names not funcs. #1650
Conversation
| std::cout << " {\n"; | ||
| std::cout << " BinaryenFunctionRef funcs[] = { "; | ||
| for (BinaryenIndex i = 0; i < numFuncs; i++) { | ||
| std::cout << " const char* funcNames[] = { "; |
There was a problem hiding this comment.
btw char* funcNames vs char *funcNames pointer placement isn't consistent in the codebase but I tried to be consistent with other similar contexts. Happy to follow any pattern.
There was a problem hiding this comment.
Yeah, we should be more consistent. I think we prefer char* funcNames (keep the type all together).
jayphelps
force-pushed
the
fun-table
branch
3 times, most recently
from
3c9e841 to
b1b0180
Compare
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Thanks! Overall looks very good.
I think we also need to update src/js/binaryen.js-post.js for the JS API.
CHANGELOG.md
Outdated
|
|
||
| ### BREAKING CHANGES | ||
|
|
||
| - `BinaryenSetFunctionTable` no longer accepts an array of functions, instead it accepts an array of function names, `const char **funcNames`. Previously, you could not include imported functions because they are of type `BinaryenImportRef` instead of `BinaryenFunctionRef`. [#1650](https://github.com/WebAssembly/binaryen/pull/1650) |
There was a problem hiding this comment.
please mention that this is in the C API
| std::cout << " {\n"; | ||
| std::cout << " BinaryenFunctionRef funcs[] = { "; | ||
| for (BinaryenIndex i = 0; i < numFuncs; i++) { | ||
| std::cout << " const char* funcNames[] = { "; |
There was a problem hiding this comment.
Yeah, we should be more consistent. I think we prefer char* funcNames (keep the type all together).
src/js/binaryen.js-post.js
Outdated
| return Module['_BinaryenSetFunctionTable']( | ||
| module, | ||
| i32sToStack( | ||
| funcNames.map(function(funcName) { |
There was a problem hiding this comment.
can use strToStack here (see examples above in the same file)
|
Looks like the test failure is in the JS API code - the kitchen sink test fails on |
jayphelps
force-pushed
the
fun-table
branch
2 times, most recently
from
97c1e3f to
4a6a2f7
Compare
|
Updated. I had needed to rebuild the js files. 4a6a2f7 feel free to squash after review. |
src/js/binaryen.js-post.js
Outdated
| }; | ||
|
|
||
| // Obtains name for a 'Function' | ||
| Module['getFunctionName'] = function(func) { |
There was a problem hiding this comment.
why was this added? I think the getXInfo functions are enough, you can get the name that way - otherwise we'd have a great many such small functions to add.
There was a problem hiding this comment.
Added for parity with the other kitchen sink demo but also cause the general info call defers to a number of other calls and I figured you’d recommend exposing this one lol. No problem! Changing 🤙
|
Great, thanks! Btw, I'm not opposed to adding new functions like that - just I was surprised it was here, I thought I'd missed something. Can discuss that in a separate issue if you want. |
2 participants
fixes #1645.
I included a changelog file per request.