[embind] Simplify createNamedFunction. NFC #20479
Conversation
Further simplification of createNamedFunction on top of emscripten-core#18748: we can avoid extra JS wrapper by setting `name` of the function directly. This is only supported via "configuring" it by `Object.defineProperty` rather than plain assignment, but otherwise has exactly same effect - it sets debug name of the function in-place. Also, ever since emscripten-core#18748 switched away from `eval`, there is no requirement for the function name to be "legal" - we can set it to the plain demangled string, making debug names even more readable.
sbc100
changed the title
src/embind/embind.js
Outdated
| }[name]; | ||
| return Object.defineProperty(body, 'name', { | ||
| value: name | ||
| }); |
There was a problem hiding this comment.
Can you do this whole thing with an arrow function. skipping the return and the outer curly braces?
There was a problem hiding this comment.
Does the old version of this function return a new object while the new version mutates the function in place, is that right/ok?
There was a problem hiding this comment.
As far as I can tell from usages, it should be fine - we only use it with newly created function which is why we want to have a unique name.
There was a problem hiding this comment.
Can you do this whole thing with an arrow function. skipping the return and the outer curly braces?
Hm arrow functions for libraries seem somewhat rare in the codebase generally. If it's something we want more of, maybe worth running a separate codemod step over all libs?
There was a problem hiding this comment.
Changed this one to arrow function for now.
There was a problem hiding this comment.
They should be fairly common these days. I've been converting them over time, where possible.
There was a problem hiding this comment.
Yeah I just meant we should probably do a single codemod run (see my ast-grep comment in the other PR).
Slight improvement of createNamedFunction on top of #18748: we can avoid extra JS trampoline by setting
nameof the function directly.This is only supported via "configuring" it by
Object.definePropertyrather than plain assignment, but otherwise has exactly same effect - it sets debug name of the function in-place.Also, ever since #18748 switched away from
eval, there is no requirement for the function name to be "legal" - we can set it to the plain demangled string, making debug names even more readable.