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

src: allow generic C++ callables in SetImmediate() by addaleax · Pull Request #28704 · nodejs/node · GitHub

/ node Public

src: allow generic C++ callables in SetImmediate() - #28704

Closed
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:native-immediate-type-erasure
ConversationCommits (1)ChecksFiles changed
Closed

src: allow generic C++ callables in SetImmediate()#28704
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:native-immediate-type-erasure

Conversation

addaleax commented Jul 15, 2019

Copy link
Copy Markdown
Member

Modify the native SetImmediate() functions to take generic C++
callables as arguments. This makes passing arguments to the callback
easier, and in particular, it allows passing std::unique_ptrs
directly, which in turn makes sure that the data they point to is
deleted if the Environment is torn down before the callback can run.

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

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jul 15, 2019

nodejs-github-bot commented Jul 15, 2019

Copy link
Copy Markdown
Collaborator

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

addaleax added the wip Issues and PRs that are still a work in progress. label Jul 15, 2019
addaleax force-pushed the native-immediate-type-erasure branch from ff700c2 to 0386696 Compare July 15, 2019 22:35

nodejs-github-bot commented Jul 15, 2019

Copy link
Copy Markdown
Collaborator

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

addaleax force-pushed the native-immediate-type-erasure branch from 0386696 to 3cf4e25 Compare July 20, 2019 15:25
Fishrock123 reviewed Jul 24, 2019
View reviewed changes
Comment thread src/node_perf.cc Outdated

Fishrock123 Jul 24, 2019

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

Why does this need mutable?

addaleax Jul 24, 2019

Copy link
Copy Markdown
Member 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

Because here entry is moved again out of the function, and so it needs to be modifiable – Do you think a comment would help/be necessary here?

addaleax commented Jul 24, 2019

Copy link
Copy Markdown
Member Author

Fwiw, I requested access to CI machines for debugging in nodejs/build#1875 because I really can’t figure out why this would crash, although it’s (probably?) a problem with the relatively overseeable N-API bits here.

nodejs-github-bot commented Jul 26, 2019

Copy link
Copy Markdown
Collaborator

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

src: allow generic C++ callables in SetImmediate()
9d04047 
Modify the native `SetImmediate()` functions to take generic C++
callables as arguments. This makes passing arguments to the callback
easier, and in particular, it allows passing `std::unique_ptr`s
directly, which in turn makes sure that the data they point to is
deleted if the `Environment` is torn down before the callback can run.
addaleax force-pushed the native-immediate-type-erasure branch from 6c0712f to 9d04047 Compare July 26, 2019 20:12

nodejs-github-bot commented Jul 26, 2019

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jul 27, 2019

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jul 28, 2019

Copy link
Copy Markdown
Collaborator

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

addaleax added review wanted PRs that need reviews. and removed wip Issues and PRs that are still a work in progress. labels Jul 28, 2019

nodejs-github-bot commented Jul 28, 2019

Copy link
Copy Markdown
Collaborator

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

addaleax commented Jul 28, 2019

Copy link
Copy Markdown
Member Author

CI is yellow, so I think this could use a review :)

addaleax commented Jul 31, 2019

Copy link
Copy Markdown
Member Author

Landed in 5207dec

addaleax closed this Jul 31, 2019
addaleax deleted the native-immediate-type-erasure branch July 31, 2019 23:41
addaleax added a commit that referenced this pull request Jul 31, 2019
src: allow generic C++ callables in SetImmediate()
5207dec 
Modify the native `SetImmediate()` functions to take generic C++
callables as arguments. This makes passing arguments to the callback
easier, and in particular, it allows passing `std::unique_ptr`s
directly, which in turn makes sure that the data they point to is
deleted if the `Environment` is torn down before the callback can run.

PR-URL: #28704
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Aug 2, 2019
src: allow generic C++ callables in SetImmediate()
d601a0a 
Modify the native `SetImmediate()` functions to take generic C++
callables as arguments. This makes passing arguments to the callback
easier, and in particular, it allows passing `std::unique_ptr`s
directly, which in turn makes sure that the data they point to is
deleted if the `Environment` is torn down before the callback can run.

PR-URL: #28704
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR mentioned this pull request Aug 6, 2019
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

jasnell jasnell approved these changes
+1 more reviewer
Fishrock123 Fishrock123 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. review wanted PRs that need reviews.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL