small cleanup of net#4227
Conversation
There was a problem hiding this comment.
You might as well merge this whole set of ifs into a single one:
if ((!pszDest && (IsLocal(addrConnect) ||
FindNode((CNetAddr)addrConnect) || CNode::IsBanned(addrConnect) ||
FindNode(addrConnect.ToStringIPPort().c_str()))) ||
(pszDest && FindNode(pszDest)))
{
return false;
}
There was a problem hiding this comment.
I'm not even sure, if anyone is interested in that pull ;).
|
Reviewed code, looks good to me other than a nit noted above. |
There was a problem hiding this comment.
Either you use:
if condition1 {
...
} else if condition2
...;
or
if condition1
{
...
}
else if condition2
...;
... but not a mix of both.
There was a problem hiding this comment.
Updated and thanks for mentioning it.
|
@sipa ping |
|
Is anyone willing to merge this or not? |
There was a problem hiding this comment.
please don't combine logic changes with style cleanups (although this one is pretty obviously correct)
There was a problem hiding this comment.
Should I create an additional commit for this or another pull?
There was a problem hiding this comment.
Splitting it to a seperate commit in this pull is fine! just don't do it in the same commit
|
ACK apart from my nit |
There was a problem hiding this comment.
Don't remove this initialization. It changes the meaning of the function, so should not be combined with style changes.
- remove an unneded else in ConnectNode() - make 0 a double and change to 0.0 in ConnectNode() - rename strDest to pszDest in OpenNetworkConnection() - remove an unneded call to our REF() macro in BindListenPort() - small style cleanups and removal of unneeded new-lines
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/1bf2ab8e9f3eb5ca89536dca5a581004df7dd2dc for binaries and test log. |
5 participants
1st commit:
2nd commit: