Skip to content

src: changing node_file's usage of v8::Resolver #18765

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.

Sign up for GitHub

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

Jump to bottom
Closed
MSLaguana wants to merge 1 commit into from
Closed

src: changing node_file's usage of v8::Resolver #18765

MSLaguana wants to merge 1 commit into from

Conversation

@MSLaguana
Copy link
Contributor

@MSLaguana MSLaguana commented Feb 13, 2018

node_file was casting back and forth between v8::Promise::Resolver and v8::Promise
This is unnecessary; most of the time it just wants the v8::Promise::Resolver,
converting to the v8::Promise only as a return value.

This was causing issues for node-chakracore, since the v8 shim that we use there did not have v8::Promise as pointer-convertible with v8::Promise::Resolver.

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

src

@MSLaguana
src: changing node_file's usage of v8::Resolver
383ee1c 
node_file was casting back and forth between v8::Resolver and v8::Promise
This is unnecessary; most of the time it just wants the v8::Resolver,
converting to the v8::Promise only as a return value.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. labels Feb 13, 2018
apapirovski
apapirovski approved these changes Feb 13, 2018
View reviewed changes
Copy link
Contributor

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

@apapirovski apapirovski requested a review from jasnell February 13, 2018 22:04
@kfarnung
Copy link
Contributor

kfarnung commented Feb 15, 2018 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/13185/
https://ci.nodejs.org/job/node-test-pull-request/13187/

@kfarnung
Copy link
Contributor

kfarnung commented Feb 16, 2018

The only remaining CI failure is: nodejs/build#1126

This is unrelated to the change.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 16, 2018
@kfarnung
Copy link
Contributor

kfarnung commented Feb 16, 2018

Landed in 197258b

@kfarnung kfarnung closed this Feb 16, 2018
kfarnung pushed a commit that referenced this pull request Feb 16, 2018
@MSLaguana @kfarnung
src: changing node_file's usage of v8::Resolver
197258b 
node_file was casting back and forth between v8::Resolver and
v8::Promise. This is unnecessary; most of the time it just wants the
v8::Resolver, converting to the v8::Promise only as a return value.

PR-URL: #18765
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
@digitalinfinity digitalinfinity mentioned this pull request Feb 20, 2018
Merged
2 tasks
@MylesBorins
Copy link
Contributor

MylesBorins commented Feb 21, 2018

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@MSLaguana MSLaguana deleted the fixFilePromiseUsage branch February 21, 2018 16:37
@MSLaguana
Copy link
Contributor Author

MSLaguana commented Feb 21, 2018

With the current state of v9.x-staging this change doesn't apply, but if 7154bc0#diff-fc4b9abe619d933ad78b886bab48caf3 is being backported then this change should go with it. How can I tell whether the other commit is being backported?

@MSLaguana
Copy link
Contributor Author

MSLaguana commented Feb 21, 2018

Ah; I see its PR lists the don't-land-on-v9.x label, so this should be the same.

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@MSLaguana @MayaLekova
src: changing node_file's usage of v8::Resolver
e424697 
node_file was casting back and forth between v8::Resolver and
v8::Promise. This is unnecessary; most of the time it just wants the
v8::Resolver, converting to the v8::Promise only as a return value.

PR-URL: nodejs#18765
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

+2 more reviewers

@apapirovski apapirovski apapirovski approved these changes

@kfarnung kfarnung kfarnung approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@MSLaguana @kfarnung @MylesBorins @apapirovski @jasnell @cjihrig @BridgeAR @nodejs-github-bot