small cleanup of net #4227

Diapolo commented at 9:25 AM on May 24, 2014: none

1st commit:

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

2nd commit:

convert an if into an else if in OpenNetworkConnection()

in src/net.cpp:None in 2f2d1f323e outdated

1433 |              FindNode((CNetAddr)addrConnect) || CNode::IsBanned(addrConnect) ||
1434 |              FindNode(addrConnect.ToStringIPPort().c_str()))
1435 |              return false;
1436 | -    if (strDest && FindNode(strDest))
1437 | +    }
1438 | +    else if (FindNode(pszDest))

brandondahler commented at 3:16 AM on May 30, 2014:

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;
}

Diapolo commented at 10:02 AM on May 31, 2014:

I'm not even sure, if anyone is interested in that pull ;).

brandondahler commented at 3:25 AM on May 30, 2014: contributor

Reviewed code, looks good to me other than a nit noted above.

in src/net.cpp:None in 2f2d1f323e outdated

1432 |          if (IsLocal(addrConnect) ||
1433 |              FindNode((CNetAddr)addrConnect) || CNode::IsBanned(addrConnect) ||
1434 |              FindNode(addrConnect.ToStringIPPort().c_str()))
1435 |              return false;
1436 | -    if (strDest && FindNode(strDest))
1437 | +    }

sipa commented at 12:03 PM on May 31, 2014:

Either you use:

if condition1 {
  ...
} else if condition2
  ...;

or

if condition1
{
  ...
}
else if condition2
  ...;

... but not a mix of both.

Diapolo commented at 7:28 AM on June 3, 2014:

Updated and thanks for mentioning it.

Diapolo commented at 3:51 PM on June 5, 2014: none

@sipa ping

Diapolo commented at 8:44 AM on June 11, 2014: none

Is anyone willing to merge this or not?

in src/net.cpp:None in b838a1a9bd outdated

1432 |          if (IsLocal(addrConnect) ||
1433 |              FindNode((CNetAddr)addrConnect) || CNode::IsBanned(addrConnect) ||
1434 |              FindNode(addrConnect.ToStringIPPort().c_str()))
1435 |              return false;
1436 | -    if (strDest && FindNode(strDest))
1437 | +    } else if (FindNode(pszDest))

laanwj commented at 9:15 AM on June 11, 2014:

please don't combine logic changes with style cleanups (although this one is pretty obviously correct)

Diapolo commented at 10:07 AM on June 11, 2014:

Should I create an additional commit for this or another pull?

laanwj commented at 10:14 AM on June 11, 2014:

Splitting it to a seperate commit in this pull is fine! just don't do it in the same commit

laanwj commented at 9:16 AM on June 11, 2014: member

ACK apart from my nit

small cleanup of net

- 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

5bd6c31bd6

convert an if into an else if in OpenNetworkConnection() 634bd61b76

in src/net.cpp:None in b838a1a9bd outdated

1561 | @@ -1566,7 +1562,6 @@ void ThreadMessageHandler()
1562 |  
1563 |  bool BindListenPort(const CService &addrBind, string& strError)
1564 |  {
1565 | -    strError = "";

laanwj commented at 9:17 AM on June 11, 2014:

Don't remove this initialization. It changes the meaning of the function, so should not be combined with style changes.

Diapolo commented at 10:10 AM on June 11, 2014:

Reverted this change!

BitcoinPullTester commented at 10:51 AM on June 11, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/1bf2ab8e9f3eb5ca89536dca5a581004df7dd2dc for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

laanwj merged this on Jun 11, 2014

laanwj closed this on Jun 11, 2014

laanwj referenced this in commit 9c8d2f6df0 on Jun 11, 2014

Diapolo deleted the branch on Jun 11, 2014

DrahtBot locked this on Sep 8, 2021