Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/binaryen-c.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2077,6 +2077,20 @@ BinaryenExpressionRef BinaryenExpressionCopy(BinaryenExpressionRef expr,
BinaryenModuleRef module) {
return ExpressionManipulator::copy(expr, *(Module*)module);
}
char* BinaryenExpressionAllocateAndWriteText(BinaryenExpressionRef expr) {
std::ostringstream os;
bool colors = Colors::isEnabled();

Colors::setEnabled(false); // do not use colors for writing
os << *(Expression*)expr;
Colors::setEnabled(colors); // restore colors state

auto str = os.str();
const size_t len = str.length() + 1;
char* output = (char*)malloc(len);
std::copy_n(str.c_str(), len, output);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

copy_n is a little strange for copying bytes, can we use strncpy or memcpy instead to convey the intent better?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree with @stevenfontanella , but yeah, let's keep it consistent here. We can simplify all these copy_n uses separately.

return output;
}

// Specific expression utility

Expand Down
5 changes: 5 additions & 0 deletions src/binaryen-c.h
Original file line number Diff line number Diff line change
Expand Up @@ -1218,6 +1218,11 @@ BINARYEN_API void BinaryenExpressionFinalize(BinaryenExpressionRef expr);
// Makes a deep copy of the given expression.
BINARYEN_API BinaryenExpressionRef
BinaryenExpressionCopy(BinaryenExpressionRef expr, BinaryenModuleRef module);
// 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);

// Block

Expand Down
10 changes: 4 additions & 6 deletions src/js/binaryen.js-post.js
Original file line number Diff line number Diff line change
Expand Up @@ -3308,12 +3308,10 @@ Module['emitText'] = function(expr) {
if (typeof expr === 'object') {
return expr.emitText();
}
const old = out;
let ret = '';
out = x => { ret += x + '\n' };
Module['_BinaryenExpressionPrint'](expr);
out = old;
return ret;
const textPtr = BinaryenObj["_BinaryenExpressionAllocateAndWriteText"](expr);
const text = UTF8ToString(textPtr);
if (textPtr) _free(textPtr);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

@chharvey chharvey May 19, 2026

Choose a reason for hiding this comment

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

sure, i again copied from the emitText for Modules but i can make the same updates to both.

Copy link
Copy Markdown
Author

@chharvey chharvey May 20, 2026

Choose a reason for hiding this comment

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

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

return text;
};

// Calls a function, wrapping it in error handling code so that if it hits a
Expand Down