Skip to content

Conversation

addaleax
Copy link
Member

Ref: #14379

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

n-api

/cc @nodejs/n-api

@addaleax addaleax added dont-land-on-v4.x node-api Issues and PRs related to the Node-API. labels Jul 20, 2017
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. labels Jul 20, 2017
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM. Are there already tests for these code paths?

@mscdex mscdex added the performance Issues and PRs related to the performance of Node.js. label Jul 20, 2017
Copy link
Contributor

@XadillaX XadillaX left a comment

Choose a reason for hiding this comment

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

Any test case or performance test?

@addaleax
Copy link
Member Author

Both the fast paths and the slow paths are already tested by addons-napi/test_conversions. Regarding performance, I didn’t explicitly measure, but judging from #14379 this just makes a lot of sense.

@addaleax
Copy link
Member Author

@addaleax
Copy link
Member Author

Landed in 57a4ceb

@addaleax addaleax closed this Jul 24, 2017
@addaleax addaleax deleted the napi-get-fast-paths branch July 24, 2017 15:36
addaleax added a commit that referenced this pull request Jul 24, 2017
Ref: #14379
PR-URL: #14393
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jason Ginchereau <[email protected]>
addaleax added a commit that referenced this pull request Jul 24, 2017
Ref: #14379
PR-URL: #14393
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jason Ginchereau <[email protected]>
@addaleax addaleax mentioned this pull request Jul 24, 2017
@addaleax addaleax mentioned this pull request Aug 2, 2017
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
Ref: nodejs#14379
PR-URL: nodejs#14393
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jason Ginchereau <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
Ref: #14379
Backport-PR-URL: #19447
PR-URL: #14393
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jason Ginchereau <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants