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: remove unused GetHostByNameWrap #17860

Closed
wants to merge 2 commits into from
Closed

src: remove unused GetHostByNameWrap #17860

wants to merge 2 commits into from

Conversation

bnoordhuis
Copy link
Member

It was a wrapper for ares_gethostbyname() that I'm unsure about if it was ever exposed at the binding layer, let alone the public API.

With the removal of GetHostByNameWrap in the previous commit, there is only one remaining call site [ed: of HostentToAddresses()]. Inlining it there lets us simplify the logic.

@bnoordhuis
src: remove unused GetHostByNameWrap
a203265 
It was a wrapper for `ares_gethostbyname()` that I'm unsure about if
it was ever exposed at the binding layer, let alone the public API.
@bnoordhuis
src: inline HostentToAddresses()
543e1fa 
With the removal of `GetHostByNameWrap` in the previous commit, there
is only one remaining call site.  Inlining it there lets us simplify
the logic.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. labels Dec 25, 2017
maclover7
maclover7 approved these changes Dec 25, 2017
View reviewed changes
Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

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

LGTM

Some fun git blame -- class became unused via e0a8e1b ;)

@tniessen
Copy link
Member

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

@jasnell
Copy link
Member

jasnell commented Dec 28, 2017

There's one CI failure I haven't seen before. Should be unrelated but there's not enough detail to confirm. @bnoordhuis can you take a look

@maclover7
Copy link
Contributor

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

@maclover7
Copy link
Contributor

Landing...

@maclover7 maclover7 self-assigned this Jan 4, 2018
maclover7 pushed a commit that referenced this pull request Jan 4, 2018
@bnoordhuis @maclover7
src: remove unused GetHostByNameWrap
a0cc20e 
It was a wrapper for `ares_gethostbyname()` that I'm unsure about if
it was ever exposed at the binding layer, let alone the public API.

PR-URL: #17860
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
maclover7 pushed a commit that referenced this pull request Jan 4, 2018
@bnoordhuis @maclover7
src: inline HostentToAddresses()
f89ee06 
With the removal of `GetHostByNameWrap` in the previous commit, there
is only one remaining call site.  Inlining it there lets us simplify
the logic.

PR-URL: #17860
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@maclover7
Copy link
Contributor

Landed in a0cc20e, f89ee06

@maclover7 maclover7 closed this Jan 4, 2018
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@bnoordhuis @MylesBorins
src: remove unused GetHostByNameWrap
95358f8 
It was a wrapper for `ares_gethostbyname()` that I'm unsure about if
it was ever exposed at the binding layer, let alone the public API.

PR-URL: #17860
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@bnoordhuis @MylesBorins
src: inline HostentToAddresses()
4c5b68b 
With the removal of `GetHostByNameWrap` in the previous commit, there
is only one remaining call site.  Inlining it there lets us simplify
the logic.

PR-URL: #17860
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@bnoordhuis @MylesBorins
src: remove unused GetHostByNameWrap
4b7e5fe 
It was a wrapper for `ares_gethostbyname()` that I'm unsure about if
it was ever exposed at the binding layer, let alone the public API.

PR-URL: #17860
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@bnoordhuis @MylesBorins
src: inline HostentToAddresses()
e0e7a16 
With the removal of `GetHostByNameWrap` in the previous commit, there
is only one remaining call site.  Inlining it there lets us simplify
the logic.

PR-URL: #17860
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@bnoordhuis @MylesBorins
src: remove unused GetHostByNameWrap
87b336a 
It was a wrapper for `ares_gethostbyname()` that I'm unsure about if
it was ever exposed at the binding layer, let alone the public API.

PR-URL: #17860
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@bnoordhuis @MylesBorins
src: inline HostentToAddresses()
98dc554 
With the removal of `GetHostByNameWrap` in the previous commit, there
is only one remaining call site.  Inlining it there lets us simplify
the logic.

PR-URL: #17860
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
Merged
@MylesBorins
Copy link
Contributor

Does this make sense for LTS?

@MylesBorins
Copy link
Contributor

ping re: backport

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

@gibfahn gibfahn gibfahn approved these changes

@tniessen tniessen tniessen approved these changes

@maclover7 maclover7 maclover7 approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

Assignees

@maclover7 maclover7

Labels
c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@bnoordhuis @tniessen @jasnell @maclover7 @MylesBorins @cjihrig @gibfahn @nodejs-github-bot