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: use local isolate instead of args.GetIsolate #14768

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@danbev
Member

danbev commented Aug 11, 2017

While stepping though SetupPromises I noticed that the environments
Isolate is used but not when creating the string "_setupPromises".

Is there a reason for using args.GetIsolate() instead of using the
environments isolate? I see that GetIsolate() is an inline call, but
could there be situations where it returns a different Isolate?
If not perhaps using the local isolate variable would be a litte
clearer.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

src: use local isolate instead of args.GetIsolate
While stepping though SetupPromises I noticed that the environments
Isolate is used but not when creating the string "_setupPromises".

Is there a reason for using args.GetIsolate() instead of using the
environments isolate? I see that GetIsolate() is an inline call, but
could there be situations where it returns a different Isolate?
If not perhaps using the local isolate variable would be a litte
clearer.
@aqrln

aqrln approved these changes Aug 11, 2017

@addaleax

This comment has been minimized.

Show comment
Hide comment
Member

addaleax commented Aug 11, 2017

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Aug 11, 2017

Member

This doesn’t need to wait 48 hours, landed in c27360e

Member

addaleax commented Aug 11, 2017

This doesn’t need to wait 48 hours, landed in c27360e

@addaleax addaleax closed this Aug 11, 2017

addaleax added a commit that referenced this pull request Aug 11, 2017

src: use local isolate instead of args.GetIsolate
While stepping though SetupPromises I noticed that the environments
Isolate is used but not when creating the string "_setupPromises".

Is there a reason for using args.GetIsolate() instead of using the
environments isolate? I see that GetIsolate() is an inline call, but
could there be situations where it returns a different Isolate?
If not perhaps using the local isolate variable would be a litte
clearer.

PR-URL: #14768
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

icarter09 added a commit to icarter09/node that referenced this pull request Aug 12, 2017

src: use local isolate instead of args.GetIsolate
While stepping though SetupPromises I noticed that the environments
Isolate is used but not when creating the string "_setupPromises".

Is there a reason for using args.GetIsolate() instead of using the
environments isolate? I see that GetIsolate() is an inline call, but
could there be situations where it returns a different Isolate?
If not perhaps using the local isolate variable would be a litte
clearer.

PR-URL: nodejs#14768
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

addaleax added a commit that referenced this pull request Aug 13, 2017

src: use local isolate instead of args.GetIsolate
While stepping though SetupPromises I noticed that the environments
Isolate is used but not when creating the string "_setupPromises".

Is there a reason for using args.GetIsolate() instead of using the
environments isolate? I see that GetIsolate() is an inline call, but
could there be situations where it returns a different Isolate?
If not perhaps using the local isolate variable would be a litte
clearer.

PR-URL: #14768
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

@addaleax addaleax referenced this pull request Aug 13, 2017

Merged

v8.4.0 proposal #14811

@danbev danbev deleted the danbev:use-isolate-instead-of-GetIsolate branch Aug 16, 2017

MylesBorins added a commit that referenced this pull request Sep 19, 2017

src: use local isolate instead of args.GetIsolate
While stepping though SetupPromises I noticed that the environments
Isolate is used but not when creating the string "_setupPromises".

Is there a reason for using args.GetIsolate() instead of using the
environments isolate? I see that GetIsolate() is an inline call, but
could there be situations where it returns a different Isolate?
If not perhaps using the local isolate variable would be a litte
clearer.

PR-URL: #14768
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 19, 2017

Member

backported to 6.x. LMK if it should be backed out

Member

MylesBorins commented Sep 19, 2017

backported to 6.x. LMK if it should be backed out

@MylesBorins MylesBorins referenced this pull request Sep 20, 2017

Merged

v6.11.4 proposal #15506

@danbev

This comment has been minimized.

Show comment
Hide comment
@danbev

danbev Sep 25, 2017

Member

backported to 6.x. LMK if it should be backed out

@MylesBorins Thanks!

Member

danbev commented Sep 25, 2017

backported to 6.x. LMK if it should be backed out

@MylesBorins Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment