-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Remove most usage of withStackSave. NFC #21764
Conversation
} | ||
var rtn = __emscripten_run_on_main_thread_js(funcIndex, emAsmAddr, serializedNumCallArgs, args, sync); | ||
stackRestore(sp); | ||
return rtn; |
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.
Maybe withStackSave
is worth it for convenience in cases like this, to avoid the annoyance of a temp var?
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.
Maybe.. although I think this is the change that actually shows up as a saving in our code size tests.. so I'm tempted to keep 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.
If it's shorter that's good enough for me I guess...
The remaining usage is all in library_wasmfs.js where it make the code a fair amount more readable to continue to use `withStackSave()` See emscripten-core#21763
} | ||
var rtn = __emscripten_run_on_main_thread_js(funcIndex, emAsmAddr, serializedNumCallArgs, args, sync); | ||
stackRestore(sp); | ||
return rtn; |
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.
If it's shorter that's good enough for me I guess...
The remaining usage is all in library_wasmfs.js where it make the code a fair amount more readable to continue to use
withStackSave()
.See #21763