Skip to content

src: RegisterHandleCleanups reduce duplication - #33421

Closed
danbev wants to merge 1 commit into
nodejs:masterfrom
danbev:register-handle-cleanup-suggestion
Closed

src: RegisterHandleCleanups reduce duplication#33421
danbev wants to merge 1 commit into
nodejs:masterfrom
danbev:register-handle-cleanup-suggestion

Conversation

@danbev

@danbevdanbev commented May 15, 2020

Copy link
Copy Markdown
Contributor

This commit suggest using a lambda for the RegisterHandlerCleanup calls
in RegisterHandleCleanups.

The motivation is to reduce some duplication and to make it a little
easier to read as all of the calls pass in the same arguments, apart
from casting the uv handle.

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

@danbev
src: RegisterHandleCleanups reduce duplication
a6f8f22
This commit suggest using a lambda for the RegisterHandlerCleanup calls
in RegisterHandleCleanups.
The motivation is to reduce some duplication and to make it a little
easier to read as all of the calls pass in the same arguments, apart
from casting the uv handle.
@nodejs-github-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 15, 2020
himself65
himself65 reviewed May 15, 2020
View reviewed changes

@himself65himself65 left a comment

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.

How about using #define macro? I worry about the performance reduce of lambda

#defineV(hook) \
RegisterHandleCleanup( \
reinterpret_cast<uv_handle_t*>(hook), close_and_finish, nullptr)
V(timer_handle());
V(immediate_check_handle());
V(immediate_idle_handle());
V(&idle_prepare_handle_);
V(&idle_check_handle_);
V(&task_queues_async_);
#undef V

addaleax
addaleax approved these changes May 15, 2020
View reviewed changes

@addaleaxaddaleax left a comment

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.

LGTM and please, no macro :)

@addaleax

addaleax commented May 15, 2020

Copy link
Copy Markdown
Member

@himself65 This code is only run once per Environment, so performance is a non-concern, but generally lambdas will perform on par with macros, so they're almost always the better choice.

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

nodejs-github-bot commented May 19, 2020

Copy link
Copy Markdown
Collaborator

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

@addaleax

addaleax commented May 19, 2020

Copy link
Copy Markdown
Member

Landed in 61189d3

addaleax pushed a commit that referenced this pull request May 19, 2020
@danbev@addaleax
src: reduce duplication in RegisterHandleCleanups
61189d3
This commit suggest using a lambda for the RegisterHandlerCleanup calls
in RegisterHandleCleanups.
The motivation is to reduce some duplication and to make it a little
easier to read as all of the calls pass in the same arguments, apart
from casting the uv handle.
PR-URL: #33421
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
codebytere pushed a commit that referenced this pull request May 21, 2020
@danbev@codebytere
src: reduce duplication in RegisterHandleCleanups
ba69511
This commit suggest using a lambda for the RegisterHandlerCleanup calls
in RegisterHandleCleanups.
The motivation is to reduce some duplication and to make it a little
easier to read as all of the calls pass in the same arguments, apart
from casting the uv handle.
PR-URL: #33421
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
codebytere pushed a commit that referenced this pull request Jun 7, 2020
@danbev@codebytere
src: reduce duplication in RegisterHandleCleanups
eb9417a
This commit suggest using a lambda for the RegisterHandlerCleanup calls
in RegisterHandleCleanups.
The motivation is to reduce some duplication and to make it a little
easier to read as all of the calls pass in the same arguments, apart
from casting the uv handle.
PR-URL: #33421
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
codebytere pushed a commit that referenced this pull request Jun 9, 2020
@danbev@codebytere
src: reduce duplication in RegisterHandleCleanups
c797c7c
This commit suggest using a lambda for the RegisterHandlerCleanup calls
in RegisterHandleCleanups.
The motivation is to reduce some duplication and to make it a little
easier to read as all of the calls pass in the same arguments, apart
from casting the uv handle.
PR-URL: #33421
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@codebyterecodebytere mentioned this pull request Jun 9, 2020
Merged
@codebyterecodebytere mentioned this pull request Jun 28, 2020
Merged
QwireyInc pushed a commit to QwireyInc/node that referenced this pull request Apr 19, 2025
@danbev@targos
src: reduce duplication in RegisterHandleCleanups
f6227c0
This commit suggest using a lambda for the RegisterHandlerCleanup calls
in RegisterHandleCleanups.
The motivation is to reduce some duplication and to make it a little
easier to read as all of the calls pass in the same arguments, apart
from casting the uv handle.
PR-URL: nodejs/node#33421
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@addaleaxaddaleaxaddaleax approved these changes
@cjihrigcjihrigcjihrig approved these changes
@richardlaurichardlaurichardlau approved these changes
@himself65himself65himself65 approved these changes
+1 more reviewer
@devnexendevnexendevnexen 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++.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@danbev@addaleax@nodejs-github-bot@jasnell@cjihrig@devnexen@richardlau@himself65