C API: Add BinaryenFunctionAppendVar#6213
C API: Add BinaryenFunctionAppendVar#6213kripken merged 2 commits intoWebAssembly:mainfrom mobsceneZ:func_add_local
Conversation
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Looks like a reasonable API to add.
See comments, + also add a tiny test somewhere in test/example/c-api-kitchen-sink.c.
src/binaryen-c.h
Outdated
| BINARYEN_API BinaryenType BinaryenFunctionGetVar(BinaryenFunctionRef func, | ||
| BinaryenIndex index); | ||
| // Appends a local variable to the specified `Function`, returning its | ||
| // insertion index. |
There was a problem hiding this comment.
| // insertion index. | |
| // index. |
src/binaryen-c.cpp
Outdated
| auto* function = (Function*)func; | ||
| auto& list = function->vars; | ||
| auto index = list.size(); | ||
| list.push_back((Type)type); |
There was a problem hiding this comment.
This can use Builder::addVar.
For consistency with that API, perhaps this could be BinaryenFunctionAddVar?
Hey kripken, according to your suggestions I made corresponding changes which can be seen in commit 7e968bd. You can check that out to see if it's what you want ;). |
src/binaryen-c.cpp
Outdated
| auto* function = (Function*)func; | ||
| return Builder::addVar(function, (Type)type); |
There was a problem hiding this comment.
| auto* function = (Function*)func; | |
| return Builder::addVar(function, (Type)type); | |
| return Builder::addVar((Function*)func, (Type)type); |
Seems like doing both casts on that line still keeps it reasonably short.
| BinaryenIndex newLocalIdx = BinaryenFunctionAddVar(sinker, BinaryenTypeFloat32()); | ||
| assert(newLocalIdx == numLocals); | ||
| assert(BinaryenFunctionGetVar(sinker, newLocalIdx - numParams) == BinaryenTypeFloat32()); | ||
|
|
There was a problem hiding this comment.
We can also check the number of vars was incremented properly.
| assert(BinaryenFunctionGetNumLocals(sinker) == numLocals + 1); | |
|
See also the error on CI about some small lint changes that are required. |
Hey Kripken, I made corresponding changes in commit b2a69dd (it's a rewrite of the last commit), you can check that out to see if that has further problems. |
2 participants
Refer to issue #6214, this pull request is trying to allow Binaryen C API to append new local variables to the function (in cases that there does not exist such C API yet).