remove GetBoolArg() fDefault parameter defaulting to false #2588

Diapolo commented at 3:39 PM on April 28, 2013: none

explicitly set the default of all GetBoolArg() calls
rework getarg_test.cpp and util_tests.cpp to cover this change
some indentation fixes

sipa commented at 3:54 PM on April 28, 2013: member

I'm not sure I like that - it makes code more readable to have the default listed explicitly, imho.

Diapolo commented at 3:58 PM on April 28, 2013: none

I just tried to harmonize the code once more, perhaps we should stop defining the default for GetBoolArg() then, which means we HAVE to specify what we want as default.

laanwj commented at 4:03 PM on April 28, 2013: member

I agree with Sipa. Explicit is better in this case, and it's not like , false adds a lot of ugly code. Personally I'd be fine with removing the default argument and specifying the default value on all GetBoolArg.

Diapolo commented at 5:07 PM on April 28, 2013: none

@laanwj @sipa Updated and removed the default for fDefault paramter.

Diapolo commented at 6:03 PM on April 28, 2013: none

Updated to include missing changes to util_tests.cpp.

in src/qt/splashscreen.cpp:None in d5ce99266c outdated

  25 | @@ -26,7 +26,7 @@
  26 |  
  27 |      // load the bitmap for writing some text over it
  28 |      QPixmap newPixmap;
  29 | -    if(GetBoolArg("-testnet")) {
  30 | +    if(fTestNet) {

Diapolo commented at 8:35 PM on April 28, 2013:

Don't merge yet, seems fTestNet is not yet set, when reaching this ;).

Diapolo commented at 8:49 PM on April 28, 2013: none

Reverted some changes, that led to wrong testnet detection because of init order (fTestNet).

BitcoinPullTester commented at 9:15 PM on April 28, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/7898e1195e73b6e9cd984b7eaa4ce0bf084f0339 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.

BitcoinPullTester commented at 4:27 PM on April 30, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/72b2dcae9e4a4b453824f80a7bd553fd928e30f7 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.

Diapolo commented at 9:08 AM on May 1, 2013: none

@sipa @laanwj Anything more to be done here?

BitcoinPullTester commented at 1:17 PM on May 3, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/b4cda6805ef3e416d76520d76da2ee09bf546583 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.

BitcoinPullTester commented at 10:46 AM on May 4, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/718589de809e59a0f6953a8f56ca46c9d7d4f8ab 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.

in src/bitcoinrpc.cpp:None in 718589de80 outdated

  38 | @@ -39,7 +39,7 @@
  39 |  
  40 |  static inline unsigned short GetDefaultRPCPort()
  41 |  {
  42 | -    return GetBoolArg("-testnet", false) ? 18332 : 8332;
  43 | +    return fTestNet ? 18332 : 8332;

laanwj commented at 6:42 PM on May 7, 2013:

For the usage in StartRPCThreads this is ok (testnet is parsed way before calling StartRPCThreads), but in the CommandLineRPC case this is incorrect, as fTestNet is never set.

Diapolo commented at 7:09 PM on May 7, 2013:

Good catch, I'll revert this change!

Diapolo commented at 7:11 PM on May 7, 2013: none

Removed from pull:

use fTestNet instead of GetBoolArg("-testnet", false), if possible (hint: needed to take care of init order)

BitcoinPullTester commented at 7:40 PM on May 7, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/9ae785bf2513f971e0578609fcd01783622f06d4 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.

BitcoinPullTester commented at 8:21 PM on May 7, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/a2771ba65f4b5902f5299140921bcaf10e3a96b2 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.

Diapolo commented at 6:52 AM on May 8, 2013: none

@sipa It's ugly to keep this updated, any meaning if this should be merged now or not?

laanwj commented at 7:40 AM on May 8, 2013: member

There is no point in trying to rush core changes. They have to be carefully reviewed and as this does not close a bug, there is not really a hurry.

laanwj commented at 7:45 PM on May 19, 2013: member

ACK after 0.8.2 release

BitcoinPullTester commented at 10:55 PM on May 24, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/86737bac3db2a8e2d068cd71354296b7740e75db 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.

remove GetBoolArg() fDefault parameter defaulting to false

- explicitly set the default of all GetBoolArg() calls
- rework getarg_test.cpp and util_tests.cpp to cover this change
- some indentation fixes
- move macdockiconhandler.h include in bitcoin.cpp to the "our headers"
  section

3260b4c090

Diapolo commented at 10:54 AM on June 1, 2013: none

Rebased, fixes merge-conflict.

in src/util.cpp:None in 3260b4c090

1255 | @@ -1257,8 +1256,8 @@ void ShrinkDebugFile()
1256 |              fclose(file);
1257 |          }
1258 |      }
1259 | -    else if(file != NULL)
1260 | -	     fclose(file);
1261 | +    else if (file != NULL)

laanwj commented at 12:20 PM on June 2, 2013:

Try to keep these kind of changes in unrelated functions out of your commits. This makes it harder to determine what actually changed.

laanwj referenced this in commit 50b4086a4a on Jun 2, 2013

laanwj merged this on Jun 2, 2013

laanwj closed this on Jun 2, 2013

BitcoinPullTester commented at 1:42 PM on June 2, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/3260b4c09006ea5c1b00c599a14e6c706ac760f8 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.

DrahtBot locked this on Sep 8, 2021