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: avoid duplicate AtExit functions #8273

Merged
merged 1 commit into from Aug 31, 2016

Conversation

Projects
None yet
7 participants
@ofrobots
Contributor

ofrobots commented Aug 25, 2016

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

src

Description of change

node.cc had two functions with the name AtExit with entirely different
purposes:

  • node::AtExit(): file static; used to register the atexit(3) handler
    for the Node process.
  • node::AtExit(void (*)(void*), void*): publicly exported symbol that
    addons can use to request callbacks upon exit.

For code readability it is better to avoid the unintentional overload.

R=@addaleax, @bnoordhuis
EDIT: CI: https://ci.nodejs.org/job/node-test-pull-request/3835/

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Aug 25, 2016

Member

LGTM

Member

addaleax commented Aug 25, 2016

LGTM

@addaleax addaleax added the lib / src label Aug 25, 2016

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Aug 25, 2016

Member

Btw, everytime I’m doing something inside node.cc I feel like I’m constantly jumping around between different parts of the file, so I’d also be fine with turning functions like these into C++ lambdas to make the code more “visually” local?

Member

addaleax commented Aug 25, 2016

Btw, everytime I’m doing something inside node.cc I feel like I’m constantly jumping around between different parts of the file, so I’d also be fine with turning functions like these into C++ lambdas to make the code more “visually” local?

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Aug 25, 2016

Contributor

@addaleax +1. However, atleast on my mac, the compiler doesn't like the function passed to atexit to be a lambda. I didn't try too hard though.

Contributor

ofrobots commented Aug 25, 2016

@addaleax +1. However, atleast on my mac, the compiler doesn't like the function passed to atexit to be a lambda. I didn't try too hard though.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Aug 25, 2016

Member

Yeah, it’s not that important anyway, was just a thought that occurred to me.

Member

addaleax commented Aug 25, 2016

Yeah, it’s not that important anyway, was just a thought that occurred to me.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 25, 2016

Member

+1 LGTM if CI is green!

Member

jasnell commented Aug 25, 2016

+1 LGTM if CI is green!

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Aug 26, 2016

Contributor

LGTM

Contributor

cjihrig commented Aug 26, 2016

LGTM

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Aug 26, 2016

Member

LGTM, one less minor irritant.

Member

bnoordhuis commented Aug 26, 2016

LGTM, one less minor irritant.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell
Member

jasnell commented Aug 30, 2016

src: avoid duplicate AtExit functions
node.cc had two functions with the name AtExit with entirely different
purposes:

* node::AtExit(): file static; used to register the atexit(3) handler
  for the Node process.
* node::AtExit(void (*)(void*), void*): publicly exported symbol that
  addons can use to request callbacks upon exit.

For code readability it is better to avoid the unintentional overload.

PR-URL: #8273
Reviewed-By: addaleax - Anna Henningsen <anna@addaleax.net>
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

@ofrobots ofrobots merged commit becbcc7 into nodejs:master Aug 31, 2016

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Aug 31, 2016

Contributor

Thanks, the new CI was good, landed as becbcc7.

Contributor

ofrobots commented Aug 31, 2016

Thanks, the new CI was good, landed as becbcc7.

@ofrobots ofrobots deleted the ofrobots:duplicate-AtExit branch Aug 31, 2016

@Fishrock123 Fishrock123 referenced this pull request Sep 6, 2016

Closed

v6.6.0 pre-proposal #8428

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 8, 2016

src: avoid duplicate AtExit functions
node.cc had two functions with the name AtExit with entirely different
purposes:

* node::AtExit(): file static; used to register the atexit(3) handler
  for the Node process.
* node::AtExit(void (*)(void*), void*): publicly exported symbol that
  addons can use to request callbacks upon exit.

For code readability it is better to avoid the unintentional overload.

PR-URL: nodejs#8273
Reviewed-By: addaleax - Anna Henningsen <anna@addaleax.net>
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

Fishrock123 added a commit that referenced this pull request Sep 9, 2016

src: avoid duplicate AtExit functions
node.cc had two functions with the name AtExit with entirely different
purposes:

* node::AtExit(): file static; used to register the atexit(3) handler
  for the Node process.
* node::AtExit(void (*)(void*), void*): publicly exported symbol that
  addons can use to request callbacks upon exit.

For code readability it is better to avoid the unintentional overload.

PR-URL: #8273
Reviewed-By: addaleax - Anna Henningsen <anna@addaleax.net>
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 30, 2016

Member

@ofrobots should this be backported? If so would you be able to submit a manual backport?

Member

MylesBorins commented Sep 30, 2016

@ofrobots should this be backported? If so would you be able to submit a manual backport?

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Sep 30, 2016

Contributor

No strong reason to backport this, specially if requires manual work :).

Contributor

ofrobots commented Sep 30, 2016

No strong reason to backport this, specially if requires manual work :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment