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: remove unused internals from node.cc #7117

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@addaleax
Member

addaleax commented Jun 2, 2016

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

src

Description of change

Remove a couple of internal wrapper methods that are neither exported in the public headers nor used internally anywhere and imho have no real value being kept around.

CI: https://ci.nodejs.org/job/node-test-commit/3632/

src: remove unused internals from node.cc
Remove a couple of internal methods that are neither exported in
the public headers nor used internally anywhere.
@jasnell

This comment has been minimized.

Member

jasnell commented Jun 2, 2016

+1 for removing stuff that's no longer needed. LGTM if CI is green and @bnoordhuis is happy with it.

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Jun 2, 2016

Nice. LGTM. CI came back green.

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Jun 3, 2016

LGTM

addaleax added a commit to addaleax/node that referenced this pull request Jun 6, 2016

src: remove unused internals from node.cc
Remove a couple of internal methods that are neither exported in
the public headers nor used internally anywhere.

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

This comment has been minimized.

Member

addaleax commented Jun 6, 2016

Landed in de4161d

@addaleax addaleax closed this Jun 6, 2016

@addaleax addaleax deleted the addaleax:remove-ununsed-internals branch Jun 6, 2016

@evanlucas

This comment has been minimized.

Member

evanlucas commented Jun 15, 2016

@addaleax Is this safe to land on v6?

@addaleax

This comment has been minimized.

Member

addaleax commented Jun 15, 2016

@evanlucas It should be, but you can feel completely free to leave it out. The point here is to clean up the code base we’re working with, whether it ends up in a v6 release or not is very likely irrelevant.

@evanlucas

This comment has been minimized.

Member

evanlucas commented Jun 15, 2016

Is there any way this would be an ABI breakage? Could some native addons be using the removed MakeCallback?

@addaleax

This comment has been minimized.

Member

addaleax commented Jun 15, 2016

Only if they declared the functions themselves in their own code, or if they included the internal headers with NODE_WANT_INTERNALS defined… It would have “please don’t do this” written all over the place. But yeah, leave it out, it doesn’t really matter anyway.

@evanlucas

This comment has been minimized.

Member

evanlucas commented Jun 15, 2016

Ok, thanks for the clarification @addaleax!

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment