Skip to content

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

Closed
addaleax wants to merge 3 commits into
nodejs:masterfrom
addaleax:onscopeleave-nostdfun
Closed

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

Conversation

@addaleax

@addaleaxaddaleax 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-botnodejs-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
devsnek reviewed Oct 26, 2019
View reviewed changes
Comment threadsrc/util.h Outdated
devsnek
devsnek approved these changes Oct 26, 2019
View reviewed changes
Comment threadsrc/util.h Outdated
@nodejs-github-bot

nodejs-github-bot commented Oct 26, 2019

Copy link
Copy Markdown
Collaborator

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

@addaleaxaddaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Oct 26, 2019
@addaleax

addaleax commented Oct 26, 2019

Copy link
Copy Markdown
MemberAuthor

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

nodejs-github-bot commented Oct 26, 2019

Copy link
Copy Markdown
Collaborator

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

@addaleaxaddaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 1, 2019
@nodejs-github-bot

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

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

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

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

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

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

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

nodejs-github-bot commented Nov 6, 2019

Copy link
Copy Markdown
Collaborator

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

@addaleax
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.
@addaleax
fixup! src: do not use std::function for OnScopeLeave
776285f
@addaleax
fixup! src: do not use std::function for OnScopeLeave
e3aa3b6
@addaleax
addaleaxforce-pushed the onscopeleave-nostdfun branch from 552b639 to e3aa3b6CompareNovember 6, 2019 21:28
@nodejs-github-bot

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

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

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

nodejs-github-bot commented Nov 7, 2019

Copy link
Copy Markdown
Collaborator

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

@addaleax

addaleax commented Nov 7, 2019

Copy link
Copy Markdown
MemberAuthor

Landed in 6072e01

@addaleaxaddaleax closed this Nov 7, 2019
@addaleax
addaleax deleted the onscopeleave-nostdfun branch November 7, 2019 09:45
addaleax added a commit that referenced this pull request Nov 7, 2019
@addaleax
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
@addaleax@MylesBorins
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>
@BridgeARBridgeAR mentioned this pull request Nov 19, 2019
Merged
@targos

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
@addaleax@MylesBorins
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>
@targostargos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@addaleax@BethGriggs
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>
@MylesBorinsMylesBorins mentioned this pull request Feb 8, 2020
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fhinkelfhinkelfhinkel approved these changes
@jasnelljasnelljasnell approved these changes
@cjihrigcjihrigcjihrig approved these changes
+2 more reviewers
@devnexendevnexendevnexen approved these changes
@devsnekdevsnekdevsnek approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs 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++.lib / srcIssues 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

@addaleax@nodejs-github-bot@targos@fhinkel@jasnell@cjihrig@devnexen@devsnek