Skip to content

src: prevent extra copies of TimerWrap::TimerCb - #40665

Merged
nodejs-github-bot merged 1 commit into
nodejs:masterfrom
RaisinTen:src/prevent-extra-copies-of-TimerWrap-TimerCb
Nov 12, 2021
Merged

src: prevent extra copies of TimerWrap::TimerCb#40665
nodejs-github-bot merged 1 commit into
nodejs:masterfrom
RaisinTen:src/prevent-extra-copies-of-TimerWrap-TimerCb

Conversation

@RaisinTen

Copy link
Copy Markdown
Member

I noticed that we were taking TimerCb as a const& and then copying
that into the member. This is completely fine when the constructor is
called with an lvalue. However, when called with an rvalue, we can allow
the std::function to be moved into the member instead of falling back
to a copy, so I changed the constructors to take in universal
references. Also, std::function constructors can take in multiple
arguments, so I further modified the constructors to use variadic
templates.

Signed-off-by: Darshan Sen darshan.sen@postman.com

@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. needs-ci PRs that need a full CI run. labels Oct 30, 2021
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@RaisinTenRaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 1, 2021
I noticed that we were taking `TimerCb` as a `const&` and then copying
that into the member. This is completely fine when the constructor is
called with an lvalue. However, when called with an rvalue, we can allow
the `std::function` to be moved into the member instead of falling back
to a copy, so I changed the constructors to take in universal
references. Also, `std::function` constructors can take in multiple
arguments, so I further modified the constructors to use variadic
templates.
Signed-off-by: Darshan Sen <darshan.sen@postman.com>
@RaisinTen
RaisinTenforce-pushed the src/prevent-extra-copies-of-TimerWrap-TimerCb branch from 6047d0a to bc7efebCompareNovember 2, 2021 15:12
@nodejs-github-bot

This comment has been minimized.

@RaisinTenRaisinTen removed the needs-ci PRs that need a full CI run. label Nov 2, 2021
@nodejs-github-bot

This comment has been minimized.

@RaisinTenRaisinTen mentioned this pull request Nov 3, 2021
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@RaisinTen

Copy link
Copy Markdown
MemberAuthor

I had to rebase on top of #40684 to fix the flaky tests. Can this get another review plz?

@RaisinTenRaisinTen added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 7, 2021
@github-actionsgithub-actionsBot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 7, 2021
@github-actionsgithub-actionsBot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Nov 7, 2021
@github-actions

Copy link
Copy Markdown
Contributor
Commit Queue failed
- Loading data for nodejs/node/pull/40665
✔ Done loading data for nodejs/node/pull/40665
----------------------------------- PR info ------------------------------------
Title src: prevent extra copies of `TimerWrap::TimerCb` (#40665)
Author Darshan Sen (@RaisinTen)
Branch RaisinTen:src/prevent-extra-copies-of-TimerWrap-TimerCb -> nodejs:master
Labels c++, lib / src, author ready
Commits 1
- src: prevent extra copies of `TimerWrap::TimerCb`
Committers 1
- Darshan Sen PR-URL: https://github.com/nodejs/node/pull/40665
Reviewed-By: Anna Henningsen Reviewed-By: Minwoo Jung ------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/40665
Reviewed-By: Anna Henningsen Reviewed-By: Minwoo Jung --------------------------------------------------------------------------------
⚠ Commits were pushed since the last review:
⚠ - src: prevent extra copies of `TimerWrap::TimerCb`
ℹ This PR was created on Sat, 30 Oct 2021 15:56:09 GMT
✔ Approvals: 2
✔ - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/40665#pullrequestreview-793747833
✔ - Minwoo Jung (@JungMinu): https://github.com/nodejs/node/pull/40665#pullrequestreview-794777797
✔ Last GitHub Actions successful
ℹ Last Full PR CI on 2021-11-03T13:44:37Z: https://ci.nodejs.org/job/node-test-pull-request/40683/
- Querying data for job/node-test-pull-request/40683/
✔ Build data downloaded
✔ Last Jenkins CI successful
--------------------------------------------------------------------------------
✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/1431079248

@aduh95aduh95 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.

RSLGTM

@aduh95aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Nov 7, 2021
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 12, 2021
@nodejs-github-bot
nodejs-github-bot merged commit 2d368da into nodejs:masterNov 12, 2021
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 2d368da

@RaisinTen
RaisinTen deleted the src/prevent-extra-copies-of-TimerWrap-TimerCb branch November 13, 2021 04:48
targos pushed a commit that referenced this pull request Nov 21, 2021
I noticed that we were taking `TimerCb` as a `const&` and then copying
that into the member. This is completely fine when the constructor is
called with an lvalue. However, when called with an rvalue, we can allow
the `std::function` to be moved into the member instead of falling back
to a copy, so I changed the constructors to take in universal
references. Also, `std::function` constructors can take in multiple
arguments, so I further modified the constructors to use variadic
templates.
Signed-off-by: Darshan Sen <darshan.sen@postman.com>
PR-URL: #40665
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@targostargos mentioned this pull request Nov 26, 2021
danielleadams pushed a commit that referenced this pull request Jan 30, 2022
I noticed that we were taking `TimerCb` as a `const&` and then copying
that into the member. This is completely fine when the constructor is
called with an lvalue. However, when called with an rvalue, we can allow
the `std::function` to be moved into the member instead of falling back
to a copy, so I changed the constructors to take in universal
references. Also, `std::function` constructors can take in multiple
arguments, so I further modified the constructors to use variadic
templates.
Signed-off-by: Darshan Sen <darshan.sen@postman.com>
PR-URL: #40665
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
I noticed that we were taking `TimerCb` as a `const&` and then copying
that into the member. This is completely fine when the constructor is
called with an lvalue. However, when called with an rvalue, we can allow
the `std::function` to be moved into the member instead of falling back
to a copy, so I changed the constructors to take in universal
references. Also, `std::function` constructors can take in multiple
arguments, so I further modified the constructors to use variadic
templates.
Signed-off-by: Darshan Sen <darshan.sen@postman.com>
PR-URL: #40665
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@danielleadamsdanielleadams mentioned this pull request Feb 1, 2022
codebytere added a commit to electron/electron that referenced this pull request Feb 10, 2022
codebytere added a commit to electron/electron that referenced this pull request Mar 9, 2022
codebytere added a commit to electron/electron that referenced this pull request Mar 17, 2022
jkleinsc pushed a commit to electron/electron that referenced this pull request Mar 23, 2022
* chore: bump node in DEPS to v16.14.0
* src: add flags for controlling process behavior
nodejs/node#40339
* src: add x509.fingerprint512 to crypto module
nodejs/node#39809
* deps: upgrade to libuv 1.43.0
nodejs/node#41398
* chore: fixup patch indices
* chore: add missing filenames
nodejs/node#39283nodejs/node#40665
* crypto: trim input for NETSCAPE_SPKI_b64_decode
nodejs/node#40757
* chore: update patches
* chore: bump node in DEPS to v16.14.1
* tools: enable no-empty ESLint rule
nodejs/node#41831
* chore: update patches
* chore: update patches
* chore: bump node in DEPS to v16.14.2
* chore: update patches
Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
bavulapati pushed a commit to bavulapati/electron that referenced this pull request Apr 29, 2022
* chore: bump node in DEPS to v16.14.0
* src: add flags for controlling process behavior
nodejs/node#40339
* src: add x509.fingerprint512 to crypto module
nodejs/node#39809
* deps: upgrade to libuv 1.43.0
nodejs/node#41398
* chore: fixup patch indices
* chore: add missing filenames
nodejs/node#39283nodejs/node#40665
* crypto: trim input for NETSCAPE_SPKI_b64_decode
nodejs/node#40757
* chore: update patches
* chore: bump node in DEPS to v16.14.1
* tools: enable no-empty ESLint rule
nodejs/node#41831
* chore: update patches
* chore: update patches
* chore: bump node in DEPS to v16.14.2
* chore: update patches
Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
* chore: bump node in DEPS to v16.14.0
* src: add flags for controlling process behavior
nodejs/node#40339
* src: add x509.fingerprint512 to crypto module
nodejs/node#39809
* deps: upgrade to libuv 1.43.0
nodejs/node#41398
* chore: fixup patch indices
* chore: add missing filenames
nodejs/node#39283nodejs/node#40665
* crypto: trim input for NETSCAPE_SPKI_b64_decode
nodejs/node#40757
* chore: update patches
* chore: bump node in DEPS to v16.14.1
* tools: enable no-empty ESLint rule
nodejs/node#41831
* chore: update patches
* chore: update patches
* chore: bump node in DEPS to v16.14.2
* chore: update patches
Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

5 participants

@RaisinTen@nodejs-github-bot@addaleax@JungMinu@aduh95