src: extracting PipeConnectWrap and TCPConnectWrap to ConnectWrap#7501
Uh oh!
There was an error while loading. Please reload this page.
Conversation
mscdex added c++ 
src/connect_wrap-inl.h Outdated
There was a problem hiding this comment.
Two nits:
- Can you use fully qualified names in headers, e.g.,
v8::FunctionCallbackInfo<v8::Value>? static inlineis tautological (in C++), justinlineis sufficient. :-)
bnoordhuis commented Jun 30, 2016
Some style issues but apart from that it's great, I like PRs that remove code duplication. |
src/connect_wrap.h Outdated
There was a problem hiding this comment.
Style nit: can you line up the arguments?
There was a problem hiding this comment.
Will do. Sorry, I was a little confused before with when to line up and when not to. Think I've got it now though, thanks for clearing that up.
bnoordhuis commented Jul 1, 2016
LGTM with final style nits. CI: https://ci.nodejs.org/job/node-test-pull-request/3149/ |
danbev commented Jul 1, 2016
@bnoordhuis Thanks for the reviews! |
bnoordhuis commented Jul 1, 2016
LGTM. One more CI: https://ci.nodejs.org/job/node-test-pull-request/3155/ |
addaleax commented Jul 4, 2016
LGTM, CI looks good except for unrelated failure. |
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 a class that are almost identical. This commit extracts these parts into a separate class that both can share.
danbevforce-pushed the extract-pipe-and-tcp-connectwrap branch from 0d3e2f5 to 952baf4Comparedanbev commented Jul 26, 2016
I've rebased this with upstream/master. |
addaleax commented Jul 26, 2016
Thanks for rebasing, CI: https://ci.nodejs.org/job/node-test-commit/4277/ |
addaleax commented Jul 28, 2016
Landed in b896057 with the subject line shortened to fit into 50 characters, thanks for the PR! |
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 a class that are almost identical. This commit extracts these parts into a separate class that both can share. PR-URL: #7501 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
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 a class that are almost identical. This commit extracts these parts into a separate class that both can share. PR-URL: #7501 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
5 participants
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected 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 a class that are almost
identical. This commit extracts these parts into a separate class
that both can share.