-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
src: pass along errors from object instantiations #25734
Conversation
Landed in 1a37fd6...8e6667f |
PR-URL: #25734 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
PR-URL: #25734 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
PR-URL: #25734 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
PR-URL: #25734 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
PR-URL: #25734 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
PR-URL: #25734 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
PR-URL: #25734 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
PR-URL: #25734 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
PR-URL: #25734 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
PR-URL: #25734 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
PR-URL: #25734 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
PR-URL: #25734 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
PR-URL: #25734 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
MaybeLocal<Object> Environment::CreateProcessObject( | ||
const std::vector<std::string>& args, | ||
const std::vector<std::string>& exec_args) { | ||
if (*TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED( |
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.
Why is this part inside CreateProcessObject
? (Or rather why is this method named CreateProcessObject
?)
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.
@joyeecheung Because it’s the part that handles args
and exec_args
, that’s pretty much all the reason … also, I called it that because that’s what it mainly does, create + set up the process object?
PR-URL: nodejs#25734 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
Calling
Function::NewInstance()
can always fail in the presence of termination exceptions, so we need to check for error conditions when calling it.This PR does not address all of those cases, but at least some of them.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes