-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
lib: restate the code to use primordials #36294
Conversation
The |
@@ -121,7 +125,7 @@ function hasUncaughtExceptionCaptureCallback() { | |||
return exceptionHandlerState.captureFn !== null; | |||
} | |||
|
|||
function noop() {} | |||
const noop = FunctionPrototype; |
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.
I don't understand why this change was made. Changing this from a function to a prototype object is changing the type entirely. Additionally, a function itself shouldn't ever need to be converted to any kind of primordial.
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.
TBF typeof Function.prototype === 'function'
, so it's not really changing the type.
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.
Interesting. Even so, I don't see the benefit of changing this unless someone can point to something specifically that this would be protecting us from?
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.
https://tc39.es/ecma262/#sec-properties-of-the-function-prototype-object
The Function prototype object:
- is itself a built-in function object.
- accepts any arguments and returns undefined when invoked.
It seems to be strictly equivalent to function noop(){}
, the only difference I can think of is that it doesn't create a new Function
object and may be marginally more memory efficient.
I highly doubt there is any scenario where it would show that Node is using one or the other, so I'm neutral to this change.
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.
Well, it’s passed to setImmediate
, which is obtained from require('timers')
at the time the callback created by createOnGlobalUncaughtException
is invoked, so if user code wraps setImmediate
from timers
, it can determine whether the passed function is %Function.prototype%
.
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.
Closing based on performance concerns outlined in #38248. |
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes