FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

async_hooks: add copyHooks function by danbev · Pull Request #19391 · nodejs/node · GitHub

/ node Public

async_hooks: add copyHooks function - #19391

Closed
danbev wants to merge 1 commit into
nodejs:masterfrom
danbev:async_hooks_set_hooks
ConversationCommits (1)ChecksFiles changed
Closed

async_hooks: add copyHooks function#19391
danbev wants to merge 1 commit into
nodejs:masterfrom
danbev:async_hooks_set_hooks

Conversation

danbev commented Mar 16, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

This commit introduces a copyHooks function that can be used by
storeActiveHooks and restoreActiveHooks to remove some code duplication.

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

nodejs-github-bot added the async_hooks Issues and PRs related to the async hooks subsystem. label Mar 16, 2018

danbev commented Mar 16, 2018

Copy link
Copy Markdown
Contributor Author

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

AndreasMadsen reviewed Mar 16, 2018
View reviewed changes
Comment thread lib/internal/async_hooks.js Outdated

AndreasMadsen Mar 16, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm not sure if I like these kinds of abstractions, but for sure it seems that from and to should be switched.

addaleax Mar 16, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe call it copyHooks? Maybe it’s just my C background that sees parallels to memcpy() & co here, but it seems closer to what’s happening than setHooks?

danbev Mar 18, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I like copyHooks, I'll update the PR. Thanks

danbev Mar 18, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@AndreasMadsen I'll also changed the parameters, so it will be copyHooks(destination, source).

addaleax approved these changes Mar 16, 2018
View reviewed changes
Comment thread lib/internal/async_hooks.js Outdated

addaleax Mar 16, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe call it copyHooks? Maybe it’s just my C background that sees parallels to memcpy() & co here, but it seems closer to what’s happening than setHooks?

apapirovski approved these changes Mar 17, 2018
View reviewed changes

apapirovski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

+1 on naming it copyHooks

async_hooks: add copyHooks function
657a33d 
This commit introduces a copyHooks function that can be used by
storeActiveHooks and restoreActiveHooks to remove some code duplication.
danbev force-pushed the async_hooks_set_hooks branch from 2045c5e to 657a33d Compare March 18, 2018 10:50
danbev changed the title async_hooks: add setHooks function async_hooks: add copyHooks function Mar 18, 2018

danbev commented Mar 18, 2018

Copy link
Copy Markdown
Contributor Author

Rebased and updated CI: https://ci.nodejs.org/job/node-test-pull-request/13726/

danbev commented Mar 18, 2018

Copy link
Copy Markdown
Contributor Author
node-test-commit failure looks unrelated

console output:

06:55:35 Building addon /home/iojs/build/workspace/node-test-commit-linux/nodes/debian8-64/test/addons-napi/test_array/
06:55:35 gyp info it worked if it ends with ok
06:55:35 gyp info using node-gyp@3.6.2
06:55:35 gyp info using node@10.0.0-pre | linux | x64
06:55:35 gyp info chdir /home/iojs/build/workspace/node-test-commit-linux/nodes/debian8-64/test/addons-napi/test_array/
06:55:35 Aborted (core dumped)
06:55:35 Makefile:369: recipe for target 'test/addons-napi/.buildstamp' failed
06:55:35 make[1]: *** [test/addons-napi/.buildstamp] Error 1
06:55:35 make[1]: *** Waiting for unfinished jobs....
06:55:49

danbev commented Mar 20, 2018

Copy link
Copy Markdown
Contributor Author

Landed in 5a4a1cb.

danbev closed this Mar 20, 2018
danbev deleted the async_hooks_set_hooks branch March 20, 2018 06:48
danbev added a commit that referenced this pull request Mar 20, 2018
async_hooks: add copyHooks function
5a4a1cb 
This commit introduces a copyHooks function that can be used by
storeActiveHooks and restoreActiveHooks to remove some code duplication.

PR-URL: #19391
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
async_hooks: add copyHooks function
a7086b0 
This commit introduces a copyHooks function that can be used by
storeActiveHooks and restoreActiveHooks to remove some code duplication.

PR-URL: #19391
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
async_hooks: add copyHooks function
f0f31d0 
This commit introduces a copyHooks function that can be used by
storeActiveHooks and restoreActiveHooks to remove some code duplication.

PR-URL: #19391
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
targos mentioned this pull request Mar 20, 2018
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax approved these changes
+2 more reviewers
AndreasMadsen AndreasMadsen left review comments
apapirovski apapirovski approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

async_hooks Issues and PRs related to the async hooks subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL