-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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,test: Prepare for future V8 changes #11437
Conversation
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.
LGTM with a green CI
src/node_contextify.cc
Outdated
@@ -261,7 +264,7 @@ class ContextifyContext { | |||
Environment* env = Environment::GetCurrent(args); | |||
if (debug_context.IsEmpty()) { | |||
// Force-load the debug context. | |||
Debug::GetMirror(args.GetIsolate()->GetCurrentContext(), args[0]); | |||
Debug::SetDebugEventListener(args.GetIsolate(), DummyDebugEventListener); |
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.
You might as well use a lambda to put things closer together:
auto dummy_event_listener = [] (const Debug::EventDetails&) {};
Debug::SetDebugEventListener(args.GetIsolate(), dummy_event_listener);
Not a deal-breaker, though.
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.
Is it supported by all compilers?
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.
Yes, it's standard C++11 syntax.
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.
Changed
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.
LGTM
Debug::GetMirror is used in contextify to force loading of the debug context. There is a better way to achieve this. PR-URL: nodejs#11437 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
In later versions of V8, the stack trace will be different than our current expectations for Promise methods. PR-URL: nodejs#11437 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Debug::GetMirror is used in contextify to force loading of the debug context. There is a better way to achieve this. PR-URL: #11437 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
In later versions of V8, the stack trace will be different than our current expectations for Promise methods. PR-URL: #11437 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
In later versions of V8, the stack trace will be different than our current expectations for Promise methods. PR-URL: #11437 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Debug::GetMirror is used in contextify to force loading of the debug context. There is a better way to achieve this. PR-URL: #11437 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
In later versions of V8, the stack trace will be different than our current expectations for Promise methods. PR-URL: #11437 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Debug::GetMirror is used in contextify to force loading of the debug context. There is a better way to achieve this. PR-URL: #11437 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Debug::GetMirror is used in contextify to force loading of the debug context. There is a better way to achieve this. PR-URL: nodejs/node#11437 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Debug::GetMirror is used in contextify to force loading of the debug context. There is a better way to achieve this. PR-URL: nodejs/node#11437 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Debug::GetMirror is used in contextify to force loading of the debug context. There is a better way to achieve this. PR-URL: nodejs/node#11437 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Debug::GetMirror is used in contextify to force loading of the debug context. There is a better way to achieve this. PR-URL: nodejs/node#11437 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes/cc @nodejs/v8 @ofrobots @hashseed