Skip to content
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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

MSLaguana
Copy link
Contributor

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

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

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 <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
@digitalinfinity digitalinfinity mentioned this pull request Feb 20, 2018
Merged
2 tasks
@MylesBorins
Copy link
Contributor

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

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

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 <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@kfarnung kfarnung kfarnung approved these changes

@jasnell jasnell jasnell approved these changes

@apapirovski apapirovski apapirovski approved these changes

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