remove evil new Function in $createNamedFunction #17273
Conversation
src/embind/embind.js
Outdated
| var context = {}; | ||
| for (var prop in body) { | ||
| context[prop] = body[prop]; | ||
| }; |
There was a problem hiding this comment.
Is this the same as Object.assign(context, body)?
Is this needed? Why can't we just use this as the context like we do above on line 205?
Can you remove the DYNAMIC_EXECUTION == 0 condition above and just always use this method?
There was a problem hiding this comment.
Oh now I understand. The following really isn't helpful except to inform you on why I got here.
Even string to function or eval() lovers should be ok without the named function, even, or at least the original... Yes, I'd bet
Object.assign(context, body);does that for-loop, abstracted. I would prefer this one,{ [name]: function(arguments){return body.apply(this,arguments);}.bind(body)}[name], also may be an abstracted assign (but declared with it), since the factory 'createNamedFunction' names itself for that purpose and I don't know why else the returned function needs to be named before returning (or why it needs to be named dynamically at all, for that matter). The whole point of this exercise is to declare a wrapper for the call{[name]:body.apply(body)}[name], for reasons I'm still unsure of.
Do you know why else the createNamedFunction exists? Your library is versatile but the DYNAMIC_EXECUTION == 0 gets by without it named, so the whole returned object seems ambivalent to the name... Can it be made into a dummy so current commands don't fail for people? Leaving that out from the script seems to be @robertaboukhalil idea, leading us into the evil new Function. Maybe some other environments requires that it be named?
There was a problem hiding this comment.
Well, since it is an if statement, there is no need to worry about it failing without the dummy idea.
maybe is a pointer, still unsure of the purpose of naming, let alone just to return it into a variable
src/embind/embind.js
Outdated
| var context = {}; | ||
| for (var prop in body) { | ||
| context[prop] = body[prop]; | ||
| }; |
There was a problem hiding this comment.
Oh now I understand. The following really isn't helpful except to inform you on why I got here.
Even string to function or eval() lovers should be ok without the named function, even, or at least the original... Yes, I'd bet
Object.assign(context, body);does that for-loop, abstracted. I would prefer this one,{ [name]: function(arguments){return body.apply(this,arguments);}.bind(body)}[name], also may be an abstracted assign (but declared with it), since the factory 'createNamedFunction' names itself for that purpose and I don't know why else the returned function needs to be named before returning (or why it needs to be named dynamically at all, for that matter). The whole point of this exercise is to declare a wrapper for the call{[name]:body.apply(body)}[name], for reasons I'm still unsure of.
Do you know why else the createNamedFunction exists? Your library is versatile but the DYNAMIC_EXECUTION == 0 gets by without it named, so the whole returned object seems ambivalent to the name... Can it be made into a dummy so current commands don't fail for people? Leaving that out from the script seems to be @robertaboukhalil idea, leading us into the evil new Function. Maybe some other environments requires that it be named?
There was a problem hiding this comment.
Honestly I don't know much about the history of this function, but I imagine it makes debugging and stack tracing way more readable if you have function names that make sense to reader, and not just "anonymous" everywhere.
Anyway, I think this change is looking really good now!
|
Thanks for working on this by the way! |
|
The flake8 errors should be fixed by a merge/rebase with main? The other errors do look related. |
|
Rebase and merge your pull request commits
|
in favor of immediately identified function key object {[n]:new fn.apply(null,args)}[n]