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: extract common DoBind and DoConnect methods #22315

Closed
wants to merge 2 commits into from
Closed

src: extract common DoBind and DoConnect methods #22315

wants to merge 2 commits into from

Conversation

maclover7
Copy link
Contributor

@maclover7 maclover7 commented Aug 14, 2018 •
edited
Loading

TCPWrap::Bind and TCPWrap::Bind6 share a large amount of functionality, so a common DoBind was extracted to remove duplication. TCPWrap::Connect/TCPWrap::Connect6 follow this same pattern, so DoConnect was extracted from those two methods to also remove duplication.

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

@nodejs-github-bot
Copy link
Collaborator

@maclover7 build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/558/pipeline

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. net Issues and PRs related to the net subsystem. labels Aug 14, 2018
@jasnell
Copy link
Member

jasnell commented Aug 14, 2018

The change looks fine but can you include a quick description of the rationale in the commit log and PR description?

@maclover7
Copy link
Contributor Author

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

@BridgeAR BridgeAR requested a review from addaleax August 17, 2018 15:41
@addaleax
Copy link
Member

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

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 19, 2018
@joyeecheung
Copy link
Member

This PR needs a rebase against master to avoid the git failure in the CI.

@joyeecheung joyeecheung mentioned this pull request Aug 20, 2018
Closed
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 25, 2018
@addaleax
Copy link
Member

@maclover7 Can you rebase?

@jasnell
Copy link
Member

jasnell commented Sep 10, 2018

Ping @maclover7

@Trott Trott mentioned this pull request Nov 15, 2018
Closed
@maclover7 maclover7 force-pushed the jm-extract-tcp branch 2 times, most recently from 9482b0e to 0697900 Compare November 29, 2018 19:13
@maclover7
Copy link
Contributor Author

@addaleax @jasnell Rebased — sorry for my delay

@maclover7
src: extract common Bind method
0697900 
`TCPWrap::Bind` and `TCPWrap::Bind6` share a large amount of
functionality, so a common `Bind` was extracted to remove duplication.
addaleax
addaleax approved these changes Nov 30, 2018
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Still LGTM

@maclover7
Copy link
Contributor Author

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

@maclover7
Copy link
Contributor Author

Passing Resume Build: https://ci.nodejs.org/job/node-test-pull-request/19153/

@maclover7
Copy link
Contributor Author

Could use another review or two @nodejs/collaborators

indutny
indutny approved these changes Dec 3, 2018
View reviewed changes
Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

LGTM if CI is happy. Thanks!

src/tcp_wrap.cc Outdated
sockaddr_in addr;
int err = uv_ip4_addr(*ip_address, port, &addr);
if (family == AF_INET6 &&
!args[2]->Uint32Value(env->context()).To(&flags)) return;
Copy link
Member

Choose a reason for hiding this comment

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

Does this pass lint? I'd rather put return on a next line and add braces on this line and the line after return.

@ronkorving
Copy link
Contributor

ronkorving commented Dec 5, 2018 •
edited
Loading

I have to share a somewhat amusing observation that while the intention of this PR is to reduce code (redundancy), GitHub measures it to remove as many lines as it adds (+23 −23).

While I can appreciate the sentiment, I see this as increasing complexity in order to remove code duplication, shrinking the code base by 0 lines.

-0 on this one.

@maclover7
Copy link
Contributor Author

maclover7 commented Dec 17, 2018 •
edited by Trott
Loading

Fixed @indutny's comment

New CI: https://ci.nodejs.org/job/node-test-pull-request/19590/ ✔️

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 17, 2018
@maclover7
Copy link
Contributor Author

Landed in 4b96a2a

@maclover7 maclover7 closed this Dec 17, 2018
@maclover7 maclover7 deleted the jm-extract-tcp branch December 17, 2018 21:41
maclover7 added a commit that referenced this pull request Dec 17, 2018
@maclover7
src: extract common Bind method
4b96a2a 
`TCPWrap::Bind` and `TCPWrap::Bind6` share a large amount of
functionality, so a common `Bind` was extracted to remove duplication.

PR-URL: #22315
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 25, 2018
@maclover7 @MylesBorins
src: extract common Bind method
a4505c6 
`TCPWrap::Bind` and `TCPWrap::Bind6` share a large amount of
functionality, so a common `Bind` was extracted to remove duplication.

PR-URL: #22315
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 25, 2018
Merged
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@maclover7 @refack
src: extract common Bind method
d2654df 
`TCPWrap::Bind` and `TCPWrap::Bind6` share a large amount of
functionality, so a common `Bind` was extracted to remove duplication.

PR-URL: nodejs#22315
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Jun 14, 2019
Closed
4 tasks
BethGriggs pushed a commit to BethGriggs/node that referenced this pull request Jun 14, 2019
@maclover7 @BethGriggs
src: extract common Bind method
f6f25cf 
`TCPWrap::Bind` and `TCPWrap::Bind6` share a large amount of
functionality, so a common `Bind` was extracted to remove duplication.

Backport-PR-URL: nodejs#28222
PR-URL: nodejs#22315
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
BethGriggs pushed a commit that referenced this pull request Jun 28, 2019
@maclover7 @BethGriggs
src: extract common Bind method
07d46a8 
`TCPWrap::Bind` and `TCPWrap::Bind6` share a large amount of
functionality, so a common `Bind` was extracted to remove duplication.

Backport-PR-URL: #28222
PR-URL: #22315
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
BethGriggs pushed a commit that referenced this pull request Jul 17, 2019
@maclover7 @BethGriggs
src: extract common Bind method
0dee607 
`TCPWrap::Bind` and `TCPWrap::Bind6` share a large amount of
functionality, so a common `Bind` was extracted to remove duplication.

Backport-PR-URL: #28222
PR-URL: #22315
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Jul 17, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lundibundi lundibundi lundibundi approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@indutny indutny indutny approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. net Issues and PRs related to the net subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@maclover7 @nodejs-github-bot @jasnell @addaleax @joyeecheung @ronkorving @indutny @lundibundi @Trott @BethGriggs