Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: refactor TimerWrap class #34252

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jul 8, 2020

src: remove user_data from TimerWrap

There’s no point in having an opaque user data pointer when we’re
already using std::function.

src: refactor TimerWrap lifetime management

Move the cleanup hook into the TimerWrap, because it should
always be present when that class is being used, and
split
Stop(true) and Stop(false) into separate methods since the
actions performed by these are fully distinct.

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

There’s no point in having an opaque user data pointer when we’re
already using `std::function`.
@addaleax addaleax requested a review from a team July 8, 2020 00:10
@nodejs-github-bot 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 8, 2020
Split `Stop(true)` and `Stop(false)` into separate methods since the
actions performed by these are fully distinct.
@addaleax
Copy link
Member Author

addaleax commented Jul 8, 2020

@jasnell So it turned out there is a reason why the cleanup hook is on the TimerWrapHandle class – it’s because otherwise, the TimerWrapHandle doesn’t know whether the cleanup hook has been called or not, and will then try to call Close again once it’s being destroyed, possibly using a TimerWrap object that doesn’t exist anymore. So I’ve reverted that bit. :)

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 9, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 9, 2020

addaleax added a commit that referenced this pull request Jul 14, 2020
There’s no point in having an opaque user data pointer when we’re
already using `std::function`.

PR-URL: #34252
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
addaleax added a commit that referenced this pull request Jul 14, 2020
Split `Stop(true)` and `Stop(false)` into separate methods since the
actions performed by these are fully distinct.

PR-URL: #34252
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@addaleax
Copy link
Member Author

Landed in 18667ac...874460a

@addaleax addaleax closed this Jul 14, 2020
@addaleax addaleax deleted the timer-wrap-cleanup branch July 14, 2020 13:05
@MylesBorins
Copy link
Member

This does not land cleanly on v14.x, should this be backported?

cjihrig pushed a commit that referenced this pull request Jul 23, 2020
There’s no point in having an opaque user data pointer when we’re
already using `std::function`.

PR-URL: #34252
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
Split `Stop(true)` and `Stop(false)` into separate methods since the
actions performed by these are fully distinct.

PR-URL: #34252
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@targos targos added backported-to-v14.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v14.x labels Apr 30, 2021
targos pushed a commit that referenced this pull request May 1, 2021
There’s no point in having an opaque user data pointer when we’re
already using `std::function`.

PR-URL: #34252
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
targos pushed a commit that referenced this pull request May 1, 2021
Split `Stop(true)` and `Stop(false)` into separate methods since the
actions performed by these are fully distinct.

PR-URL: #34252
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants