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

src: do not use `std::function` for `OnScopeLeave` by addaleax · Pull Request #30134 · nodejs/node · GitHub

/ node Public

src: do not use std::function for OnScopeLeave - #30134

Closed
addaleax wants to merge 3 commits into
nodejs:masterfrom
addaleax:onscopeleave-nostdfun
ConversationCommits (3)ChecksFiles changed
Closed

src: do not use std::function for OnScopeLeave#30134
addaleax wants to merge 3 commits into
nodejs:masterfrom
addaleax:onscopeleave-nostdfun

Conversation

addaleax commented Oct 26, 2019

Copy link
Copy Markdown
Member

Using std::function adds an extra layer of indirection, and in
particular, heap allocations that are not necessary in our use case
here.

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 Oct 26, 2019
devsnek reviewed Oct 26, 2019
View reviewed changes
Comment thread src/util.h Outdated
devsnek approved these changes Oct 26, 2019
View reviewed changes
Comment thread src/util.h Outdated

nodejs-github-bot commented Oct 26, 2019

Copy link
Copy Markdown
Collaborator

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

addaleax added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Oct 26, 2019

addaleax commented Oct 26, 2019

Copy link
Copy Markdown
Member Author

Hm … looks like Windows doesn’t like this?

I’ve added a move operator for the struct, I could imagine that MSVC added a copy operation when returning the value from the OnScopeLeave() helper function. Let’s see what CI says…

nodejs-github-bot commented Oct 26, 2019

Copy link
Copy Markdown
Collaborator

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

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 1, 2019

nodejs-github-bot commented Nov 1, 2019

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Nov 1, 2019

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Nov 4, 2019

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Nov 4, 2019

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Nov 5, 2019

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Nov 5, 2019

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Nov 5, 2019

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Nov 6, 2019

Copy link
Copy Markdown
Collaborator

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

src: do not use std::function for OnScopeLeave
206f134 
Using `std::function` adds an extra layer of indirection, and in
particular, heap allocations that are not necessary in our use case
here.
fixup! src: do not use std::function for OnScopeLeave
776285f
fixup! src: do not use std::function for OnScopeLeave
e3aa3b6
addaleax force-pushed the onscopeleave-nostdfun branch from 552b639 to e3aa3b6 Compare November 6, 2019 21:28

nodejs-github-bot commented Nov 6, 2019

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Nov 6, 2019

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Nov 6, 2019

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Nov 7, 2019

Copy link
Copy Markdown
Collaborator

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

addaleax commented Nov 7, 2019

Copy link
Copy Markdown
Member Author

Landed in 6072e01

addaleax closed this Nov 7, 2019
addaleax deleted the onscopeleave-nostdfun branch November 7, 2019 09:45
addaleax added a commit that referenced this pull request Nov 7, 2019
src: do not use std::function for OnScopeLeave
6072e01 
Using `std::function` adds an extra layer of indirection, and in
particular, heap allocations that are not necessary in our use case
here.

PR-URL: #30134
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
src: do not use std::function for OnScopeLeave
058a8d5 
Using `std::function` adds an extra layer of indirection, and in
particular, heap allocations that are not necessary in our use case
here.

PR-URL: #30134
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR mentioned this pull request Nov 19, 2019
Merged

targos commented Dec 1, 2019

Copy link
Copy Markdown
Member

To land on v12.x, this should either wait for #29874, or be backported.

MylesBorins pushed a commit that referenced this pull request Jan 10, 2020
src: do not use std::function for OnScopeLeave
69dac4b 
Using `std::function` adds an extra layer of indirection, and in
particular, heap allocations that are not necessary in our use case
here.

PR-URL: #30134
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
src: do not use std::function for OnScopeLeave
a5d2b66 
Using `std::function` adds an extra layer of indirection, and in
particular, heap allocations that are not necessary in our use case
here.

PR-URL: #30134
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins mentioned this pull request Feb 8, 2020
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
cjihrig cjihrig approved these changes
+3 more reviewers
fhinkel fhinkel approved these changes
devnexen devnexen approved these changes
devsnek devsnek 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 outstanding review comments, and a CI started. 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL