Removing unnecessary comparison of size_t maxConfirms #15031

pull canselcik wants to merge 1 commits into bitcoin:master from canselcik:maxConfirms changing 1 files +1 −1

canselcik commented at 8:43 AM on December 24, 2018: none

maxConfirms here has type size_t and therefore maxConfirms < 0 will never be true even in case of a corruption. That scenario is correctly accounted for and handled by the maxConfirms > 6 * 24 * 7 condition anyway.
fanquake added the label Refactoring on Dec 24, 2018
gmaxwell commented at 10:00 PM on December 24, 2018: contributor

How will this change benefit users of the software? The compiler will optimize out a test like this when it is actually useless, and yet to the extent that the removal does anything it will simply make the code more brittle against any future change in the underlying type.

Actually looking at the code, I see that the test is actually <= 0, so this would-- in fact-- change the behaviour. So, in fact, the change AFAICT introduces a bug-- which again makes a case for not making these sorts of changes.

in src/policy/fees.cpp:435 in d8deb86c62 outdated

 431 | @@ -432,7 +432,7 @@ void TxConfirmStats::Read(CAutoFile& filein, int nFileVersion, size_t numBuckets
 432 |      maxPeriods = confAvg.size();
 433 |      maxConfirms = scale * maxPeriods;
 434 |  
 435 | -    if (maxConfirms <= 0 || maxConfirms > 6 * 24 * 7) { // one week
 436 | +    if (maxConfirms > 6 * 24 * 7) { // one week

hebasto commented at 2:48 PM on December 26, 2018:

maxConfirms == 0 test is missed even for type size_t. Agree with @gmaxwell.

promag commented at 4:57 PM on December 26, 2018: member

Agree with @gmaxwell, I think we would be happier with making the type signed.
canselcik commented at 8:53 PM on December 26, 2018: none

Thanks for the feedback guys. I agree it would be better making the type signed. Let me make the changes accordingly and update the PR.
canselcik force-pushed on Dec 26, 2018
canselcik force-pushed on Dec 26, 2018
maxConfirms has type size_t and therefore will never have a value lower than 0.
Changing type of maxConfirms and maxPeriods to ssize_t from size_t.
8c4cfcba11
canselcik force-pushed on Dec 26, 2018
canselcik commented at 11:14 PM on December 26, 2018: none

@gmaxwell @promag Updated the PR so that the type is ssize_t for maxConfirms and maxPeriods.
promag commented at 1:14 AM on December 31, 2018: member

@canselcik despite being better IMO this is an unnecessary change as there's nothing wrong with the current code.
laanwj commented at 11:41 AM on January 2, 2019: member

Closing, as there is no agreement to make any change here.
laanwj closed this on Jan 2, 2019
DrahtBot locked this on Dec 16, 2021

Contributors

Labels