src: extracting PipeConnectWrap and TCPConnectWrap to ConnectWrap#7501
src: extracting PipeConnectWrap and TCPConnectWrap to ConnectWrap#7501danbev wants to merge 9 commits intonodejs:masterfrom
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. :-)
|
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.
|
LGTM with final style nits. CI: https://ci.nodejs.org/job/node-test-pull-request/3149/ |
|
@bnoordhuis Thanks for the reviews! |
|
LGTM. One more CI: https://ci.nodejs.org/job/node-test-pull-request/3155/ |
|
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.
danbev
force-pushed
the
extract-pipe-and-tcp-connectwrap
branch
from
0d3e2f5 to
952baf4
Compare
|
I've rebased this with upstream/master. |
|
Thanks for rebasing, CI: https://ci.nodejs.org/job/node-test-commit/4277/ |
|
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 <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>
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 <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>
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.