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.

Sign up for GitHub

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

Jump to bottom

src: use std::list for at_exit_functions #12255

Closed
wants to merge 1 commit into from
Closed

src: use std::list for at_exit_functions #12255

wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Apr 6, 2017

This change was suggested by bnoordhuis in the following comment:
#9163 (comment)

Not including any tests as this is covered by test/addons/at-exit.

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

src

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 6, 2017
@danbev
Copy link
Contributor Author

danbev commented Apr 6, 2017

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

@mscdex mscdex added the process Issues and PRs related to the process subsystem. label Apr 6, 2017
bnoordhuis
bnoordhuis requested changes Apr 6, 2017
View reviewed changes
src/node.cc Outdated
void (*cb_)(void* arg);
void* arg_;
};

static AtExitCallback* at_exit_functions_;
static std::list<AtExitCallback*> at_exit_functions_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use AtExitCallback not a pointer-to-AtExitCallback. It's leaking memory now in RunAtExit().

(Tiny nit: it should really be called at_exit_functions, it's not a class member.)

src/node.cc Outdated
p->cb_(p->arg_);
delete p;
p = q;
for (AtExitCallback* atExit : at_exit_functions_) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: at_exit

bnoordhuis
bnoordhuis reviewed Apr 6, 2017
View reviewed changes
src/node.cc Outdated
p->cb_ = cb;
p->arg_ = arg;
at_exit_functions_.push_back(p);
AtExitCallback p = {cb, arg};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, maybe call this at_exit for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good point, will change that. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is using an anonymous object something considered ok to do?

at_exit_functions.push_back(AtExitCallback{cb, arg});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's fine.

@danbev danbev mentioned this pull request Apr 6, 2017
Closed
2 tasks
@danbev
Copy link
Contributor Author

danbev commented Apr 6, 2017

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

@danbev
src: use std::list for at_exit_functions
04d313a 
This change was suggested by bnoordhuis in the following comment:
nodejs#9163 (comment)

Not including any tests as this is covered by test/addons/at-exit.
@danbev
Copy link
Contributor Author

danbev commented Apr 9, 2017

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

danbev added a commit to danbev/node that referenced this pull request Apr 10, 2017
@danbev
src: use std::list for at_exit_functions
fe016c6 
This change was suggested by bnoordhuis in the following comment:
nodejs#9163 (comment)

Not including any tests as this is covered by test/addons/at-exit.

PR-URL: nodejs#12255
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@danbev
Copy link
Contributor Author

danbev commented Apr 10, 2017

Landed in fe016c6

@danbev danbev closed this Apr 10, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Apr 10, 2017
@danbev @italoacasas
src: use std::list for at_exit_functions
37568c0 
This change was suggested by bnoordhuis in the following comment:
nodejs#9163 (comment)

Not including any tests as this is covered by test/addons/at-exit.

PR-URL: nodejs#12255
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
Merged
2 tasks
@danbev danbev deleted the atexit-list branch April 13, 2017 11:31
@MylesBorins
Copy link
Contributor

Should this be cherry-picked to v6.x? It lands cleanly fwiw

@danbev
Copy link
Contributor Author

danbev commented Apr 19, 2017

@MylesBorins Not sure as it is a minor improvement, but if it lands cleanly I see no harm in that. Let me know if you'd like me to create a PR against v6.x-staging. Thanks

MylesBorins pushed a commit that referenced this pull request May 15, 2017
@danbev @MylesBorins
src: use std::list for at_exit_functions
f1ea367 
This change was suggested by bnoordhuis in the following comment:
#9163 (comment)

Not including any tests as this is covered by test/addons/at-exit.

PR-URL: #12255
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 23, 2017
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@danbev @andrew749
src: use std::list for at_exit_functions
f796270 
This change was suggested by bnoordhuis in the following comment:
nodejs/node#9163 (comment)

Not including any tests as this is covered by test/addons/at-exit.

PR-URL: nodejs/node#12255
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@richardlau richardlau richardlau approved these changes

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@danbev @MylesBorins @bnoordhuis @addaleax @cjihrig @richardlau @mscdex @nodejs-github-bot