feat: add BinaryenExpressionAllocateAndWriteText#8717
Conversation
| auto str = os.str(); | ||
| const size_t len = str.length() + 1; | ||
| char* output = (char*)malloc(len); | ||
| std::copy_n(str.c_str(), len, output); |
There was a problem hiding this comment.
copy_n is a little strange for copying bytes, can we use strncpy or memcpy instead to convey the intent better?
There was a problem hiding this comment.
I'm honestly not sure, i just copied the exact code from BinaryenModuleAllocateAndWriteText.
I'll defer to the C developers for what's best. I'm happy as long as it's consistent.
There was a problem hiding this comment.
I agree with @stevenfontanella , but yeah, let's keep it consistent here. We can simplify all these copy_n uses separately.
| // Serialize an expression in s-expression form. Implicitly allocates the returned | ||
| // char* with malloc(), and expects the user to free() them manually | ||
| // once not needed anymore. | ||
| BINARYEN_API char* BinaryenExpressionAllocateAndWriteText(BinaryenExpressionRef expr); |
There was a problem hiding this comment.
Let's add a CHANGELOG.md entry for this and unit tests in test/example/c-api-kitchen-sink.c / test/example/c-api-kitchen-sink.txt.
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
lgtm with @stevenfontanella 's comment about the changelog and testing
| auto str = os.str(); | ||
| const size_t len = str.length() + 1; | ||
| char* output = (char*)malloc(len); | ||
| std::copy_n(str.c_str(), len, output); |
There was a problem hiding this comment.
I agree with @stevenfontanella , but yeah, let's keep it consistent here. We can simplify all these copy_n uses separately.
| const textPtr = BinaryenObj["_BinaryenExpressionAllocateAndWriteText"](expr); | ||
| const text = UTF8ToString(textPtr); | ||
| if (textPtr) _free(textPtr); |
There was a problem hiding this comment.
Can we use try ... finally here to ensure that the pointer is freed even if UTF8ToString throws an exception? I can't find the definition of UTF8ToString so I'm not sure if that's possible, but it conveys the intent better especially in case this code changes.
Also, we typically use braces around if bodies, let's do that here.
There was a problem hiding this comment.
sure, i again copied from the emitText for Modules but i can make the same updates to both.
There was a problem hiding this comment.
also… digging into this project helped me learn a lot about Emscripten and where all this stuff like UTF8ToString comes from. see my .d.ts file here for a quasi-explanation. Looks like Emscripten declares it here
3 participants
adds a function for returning a string representation of an expression in s-expression format.
closes #8716