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: pull AfterConnect from pipe_wrap and tcp_wrap #8448

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@danbev
Member

danbev commented Sep 8, 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

This commit attempts to address one of the items in
#4641 which is related to
src/pipe_wrap.cc and src/tcp_wrap.cc.

Currently both pipe_wrap.cc and tcp_wrap.cc contain an AfterConnect
function that are almost identical. This commit extracts this function
into ConnectionWrap so that that both can share it.

src: pull AfterConnect from pipe_wrap and tcp_wrap
This commit attempts to address one of the items in
#4641 which is related to
src/pipe_wrap.cc and src/tcp_wrap.cc.

Currently both pipe_wrap.cc and tcp_wrap.cc contain an AfterConnect
function that are almost identical. This commit extracts this function
into ConnectionWrap so that that both can share it.
@danbev

This comment has been minimized.

Show comment
Hide comment
} else {
readable = uv_is_readable(req->handle) != 0;
writable = uv_is_writable(req->handle) != 0;
}

This comment has been minimized.

@danbev

danbev Sep 8, 2016

Member

I'm uncertain if this is alright for TCPWrap or not (TCPWrap always set these arguments to true) and wanted to open the PR to get some feedback.

@danbev

danbev Sep 8, 2016

Member

I'm uncertain if this is alright for TCPWrap or not (TCPWrap always set these arguments to true) and wanted to open the PR to get some feedback.

This comment has been minimized.

@bnoordhuis

bnoordhuis Sep 8, 2016

Member

It should be alright. The TCP sockets that libuv creates are always readable+writable.

@bnoordhuis

bnoordhuis Sep 8, 2016

Member

It should be alright. The TCP sockets that libuv creates are always readable+writable.

This comment has been minimized.

@danbev

danbev Sep 11, 2016

Member

Thanks!

@danbev

danbev Sep 11, 2016

Member

Thanks!

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Sep 8, 2016

Member

LGTM

Member

bnoordhuis commented Sep 8, 2016

LGTM

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Sep 8, 2016

Member

CI failure looks infra related and unrelated to this change.

Member

Trott commented Sep 8, 2016

CI failure looks infra related and unrelated to this change.

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Sep 9, 2016

Contributor

LGTM

Contributor

cjihrig commented Sep 9, 2016

LGTM

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 9, 2016

Member

LGTM

Member

addaleax commented Sep 9, 2016

LGTM

@thefourtheye

This comment has been minimized.

Show comment
Hide comment
@thefourtheye

thefourtheye Sep 10, 2016

Contributor

LGTM

Contributor

thefourtheye commented Sep 10, 2016

LGTM

danbev added a commit to danbev/node that referenced this pull request Sep 11, 2016

src: pull AfterConnect from pipe_wrap and tcp_wrap
This commit attempts to address one of the items in
nodejs#4641 which is related to
src/pipe_wrap.cc and src/tcp_wrap.cc.

Currently both pipe_wrap.cc and tcp_wrap.cc contain an AfterConnect
function that are almost identical. This commit extracts this function
into ConnectionWrap so that that both can share it.

PR-URL: nodejs#8448
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@danbev

This comment has been minimized.

Show comment
Hide comment
@danbev

danbev Sep 11, 2016

Member

Thanks for the reviews!

Landed in 83a354c

Member

danbev commented Sep 11, 2016

Thanks for the reviews!

Landed in 83a354c

@danbev danbev closed this Sep 11, 2016

@danbev danbev deleted the danbev:extracting-afterconnect branch Sep 11, 2016

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

src: pull AfterConnect from pipe_wrap and tcp_wrap
This commit attempts to address one of the items in
#4641 which is related to
src/pipe_wrap.cc and src/tcp_wrap.cc.

Currently both pipe_wrap.cc and tcp_wrap.cc contain an AfterConnect
function that are almost identical. This commit extracts this function
into ConnectionWrap so that that both can share it.

PR-URL: #8448
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 30, 2016

Member

should this be backported to v4?

Member

MylesBorins commented Sep 30, 2016

should this be backported to v4?

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