Use const in COutPoint class #14838

hebasto commented at 9:13 AM on November 29, 2018: member

Refactoring:

all cases of using (uint32_t) -1 in COutPoint class are replaced with const;
also all remaining instances of (UNSIGNED)-1 transformed to std::numeric_limits<UNSIGNED>::max() (by @practicalswift).

fanquake added the label Refactoring on Nov 29, 2018

in src/primitives/transaction.h:24 in 95e2c1a9ac outdated

  20 | @@ -21,7 +21,9 @@ class COutPoint
  21 |      uint256 hash;
  22 |      uint32_t n;
  23 |  
  24 | -    COutPoint(): n((uint32_t) -1) { }
  25 | +    static constexpr uint32_t NULL_OUTPOINT_N = (uint32_t) -1;

practicalswift commented at 9:25 AM on November 29, 2018:

Use std::numeric_limits<uint32_t>::max() instead :-)

practicalswift commented at 9:25 AM on November 29, 2018: contributor

Concept ACK modulo review comment

hebasto force-pushed on Nov 29, 2018

hebasto commented at 9:54 AM on November 29, 2018: member

@practicalswift Thank you for your review. Your comment has been addressed.

practicalswift commented at 10:18 AM on November 29, 2018: contributor

utACK 6a537172cb96efbbaa6cdd8d31d4938dab4b648d

Empact commented at 12:31 AM on November 30, 2018: member

nit: Maybe call it NULL_INDEX, as n is short for index, and the value is already scoped to COutPoint.

Use const in COutPoint class 6b82fc59eb

hebasto force-pushed on Nov 30, 2018

hebasto commented at 10:55 AM on November 30, 2018: member

@Empact Thank you for your review. Your comment has been addressed.

promag commented at 3:16 PM on December 3, 2018: member

utACK 6b82fc5.

practicalswift commented at 3:36 PM on December 3, 2018: contributor

utACK 6b82fc59eb19004e54f910261a40d5e1b9e44b42

MarcoFalke commented at 6:03 PM on December 3, 2018: member

utACK 6b82fc59eb

Empact commented at 11:08 AM on December 4, 2018: member

utACK 6b82fc5

practicalswift commented at 12:19 PM on December 4, 2018: contributor

@hebasto Consider cherry-picking in https://github.com/bitcoin/bitcoin/commit/35c6ba8c64de02069bc82dbcc790193f05332f78 which transforms all remaining instances of (UNSIGNED)-1 to std::numeric_limits<UNSIGNED>::max().

hebasto commented at 1:40 PM on December 4, 2018: member

@practicalswift

Consider cherry-picking in 35c6ba8 which transforms all remaining instances of (UNSIGNED)-1 to std::numeric_limits<UNSIGNED>::max().

I don't how to point my local git to 35c6ba8c64de02069bc82dbcc790193f05332f78 commit (cannot see related PR). Would you mind showing me how to do this?

Btw, what's your nick on IRC (to get in touch)?

practicalswift commented at 2:28 PM on December 4, 2018: contributor

@hebasto The easiest way to do it is probably:

curl https://github.com/bitcoin/bitcoin/commit/35c6ba8c64de02069bc82dbcc790193f05332f78.patch \
    | git am

Use std::numeric_limits<UNSIGNED>::max()) instead of (UNSIGNED)-1 cf4b0327ed

hebasto commented at 6:26 PM on December 4, 2018: member

@practicalswift

Consider cherry-picking in 35c6ba8 which transforms all remaining instances of (UNSIGNED)-1 to std::numeric_limits<UNSIGNED>::max().

Done.

practicalswift commented at 8:48 PM on December 4, 2018: contributor

utACK cf4b0327ed92ca8a1533cdf6c2b0015fd9b56397

Empact commented at 11:14 PM on December 4, 2018: member

I would drop the latter commit. The PR stands alone and has 4 acks as-was. Introducing the latter commit broadens the scope unnecessarily.

E.g. the latter commit ~is not comprehensive, and~ has no mechanism to ensure ongoing enforcement (compiler warning).

practicalswift commented at 7:52 AM on December 5, 2018: contributor

@Empact The latter commit is meant to be comprehensive: did you find any counterexample?

Empact commented at 8:39 AM on December 5, 2018: member

Ironically I found an example that was not addressed by the second commit but had already been by the first, so no I don’t know of one.

promag commented at 9:01 AM on December 5, 2018: member

utACK cf4b032, no harm in being more comprehensive.

in src/primitives/transaction.h:24 in cf4b0327ed

  20 | @@ -21,7 +21,9 @@ class COutPoint
  21 |      uint256 hash;
  22 |      uint32_t n;
  23 |  
  24 | -    COutPoint(): n((uint32_t) -1) { }
  25 | +    static constexpr uint32_t NULL_INDEX = std::numeric_limits<uint32_t>::max();

Empact commented at 8:47 AM on December 6, 2018:

Should add #include <limits> to this file

in src/test/serialize_tests.cpp:203 in cf4b0327ed

 199 | @@ -200,7 +200,7 @@ BOOST_AUTO_TEST_CASE(varints)
 200 |      }
 201 |  
 202 |      for (uint64_t i = 0;  i < 100000000000ULL; i += 999999937) {
 203 | -        uint64_t j = -1;
 204 | +        uint64_t j = std::numeric_limits<uint64_t>::max();

Empact commented at 8:47 AM on December 6, 2018:

Should add #include <limits> to this file

in src/test/sighash_tests.cpp:108 in cf4b0327ed

 104 | @@ -105,7 +105,7 @@ void static RandomTransaction(CMutableTransaction &tx, bool fSingle) {
 105 |          txin.prevout.hash = InsecureRand256();
 106 |          txin.prevout.n = InsecureRandBits(2);
 107 |          RandomScript(txin.scriptSig);
 108 | -        txin.nSequence = (InsecureRandBool()) ? InsecureRand32() : (unsigned int)-1;
 109 | +        txin.nSequence = (InsecureRandBool()) ? InsecureRand32() : std::numeric_limits<uint32_t>::max();

Empact commented at 8:48 AM on December 6, 2018:

Should add #include <limits> to this file

Empact commented at 8:49 AM on December 6, 2018: member

Fair enough! Though on second look I noticed some missing includes, as noted.

laanwj commented at 2:37 PM on December 6, 2018: member

utACK cf4b0327ed92ca8a1533cdf6c2b0015fd9b56397

laanwj merged this on Dec 6, 2018

laanwj closed this on Dec 6, 2018

laanwj referenced this in commit 127b30cce8 on Dec 6, 2018

hebasto deleted the branch on Dec 6, 2018

deadalnix referenced this in commit 4b1e7c0910 on Mar 12, 2020

ftrader referenced this in commit e636e8cf78 on May 19, 2020

PastaPastaPasta referenced this in commit 3b7b3d23cf on Jun 27, 2021

PastaPastaPasta referenced this in commit cadc497d7a on Jun 28, 2021

PastaPastaPasta referenced this in commit 4b3df9e8fd on Jun 29, 2021

PastaPastaPasta referenced this in commit 1bf14e42c8 on Jul 1, 2021

PastaPastaPasta referenced this in commit 3d0903d802 on Jul 1, 2021

PastaPastaPasta referenced this in commit 42acfa76a4 on Jul 1, 2021

UdjinM6 referenced this in commit 90871ec251 on Jul 5, 2021

PastaPastaPasta referenced this in commit b83cab5441 on Jul 8, 2021

gades referenced this in commit 56ec015e1c on Apr 23, 2022

DrahtBot locked this on Aug 16, 2022