Use const in COutPoint class #14838

hebasto · 2018-11-29T09:13:12Z

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).

src/primitives/transaction.h

practicalswift · 2018-11-29T09:25:47Z

Concept ACK modulo review comment

hebasto · 2018-11-29T09:54:57Z

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

practicalswift · 2018-11-29T10:18:04Z

utACK 6a537172cb96efbbaa6cdd8d31d4938dab4b648d

Empact · 2018-11-30T00:31:18Z

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

hebasto · 2018-11-30T10:55:05Z

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

promag · 2018-12-03T15:16:11Z

utACK 6b82fc5.

practicalswift · 2018-12-03T15:36:19Z

utACK 6b82fc5

maflcko · 2018-12-03T18:03:46Z

utACK 6b82fc5

Empact · 2018-12-04T11:08:53Z

utACK 6b82fc5

practicalswift · 2018-12-04T12:19:49Z

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

hebasto · 2018-12-04T13:40:41Z

@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 · 2018-12-04T14:28:36Z

@hebasto The easiest way to do it is probably:

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

hebasto · 2018-12-04T18:26:04Z

@practicalswift

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

Done.

practicalswift · 2018-12-04T20:48:48Z

utACK cf4b032

Empact · 2018-12-04T23:14:01Z

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 · 2018-12-05T07:52:31Z

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

Empact · 2018-12-05T08:39:24Z

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 · 2018-12-05T09:01:12Z

utACK cf4b032, no harm in being more comprehensive.

Empact · 2018-12-06T08:47:03Z

src/primitives/transaction.h

@@ -21,7 +21,9 @@ class COutPoint
    uint256 hash;
    uint32_t n;

-    COutPoint(): n((uint32_t) -1) { }
+    static constexpr uint32_t NULL_INDEX = std::numeric_limits<uint32_t>::max();


Should add #include <limits> to this file

Empact · 2018-12-06T08:47:57Z

src/test/serialize_tests.cpp

@@ -200,7 +200,7 @@ BOOST_AUTO_TEST_CASE(varints)
    }

    for (uint64_t i = 0;  i < 100000000000ULL; i += 999999937) {
-        uint64_t j = -1;
+        uint64_t j = std::numeric_limits<uint64_t>::max();


Should add #include <limits> to this file

Empact · 2018-12-06T08:48:20Z

src/test/sighash_tests.cpp

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


Should add #include <limits> to this file

Empact · 2018-12-06T08:49:06Z

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

laanwj · 2018-12-06T14:37:24Z

utACK cf4b032

@practicalswift

cf4b032 Use std::numeric_limits<UNSIGNED>::max()) instead of (UNSIGNED)-1 (practicalswift) 6b82fc5 Use const in COutPoint class (Hennadii Stepanov) Pull request description: 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). Tree-SHA512: fc7fe9838b6e5136d8b97ea3d6f64c4aaa1215f4369832df432cab017396620bb6e30520a64180ceab6de222562ac11eab243a78dfa5a658ba018835a34caa19

@practicalswift

Summary: ``` 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). ``` Backport of core [[bitcoin/bitcoin#14838 | PR14838]]. Test Plan: ninja check Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5464

@practicalswift

Summary: ``` 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). ``` Backport of core [[bitcoin/bitcoin#14838 | PR14838]]. Test Plan: ninja check Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5464

@practicalswift

cf4b032 Use std::numeric_limits<UNSIGNED>::max()) instead of (UNSIGNED)-1 (practicalswift) 6b82fc5 Use const in COutPoint class (Hennadii Stepanov) Pull request description: 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). Tree-SHA512: fc7fe9838b6e5136d8b97ea3d6f64c4aaa1215f4369832df432cab017396620bb6e30520a64180ceab6de222562ac11eab243a78dfa5a658ba018835a34caa19

@practicalswift

cf4b032 Use std::numeric_limits<UNSIGNED>::max()) instead of (UNSIGNED)-1 (practicalswift) 6b82fc5 Use const in COutPoint class (Hennadii Stepanov) Pull request description: 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). Tree-SHA512: fc7fe9838b6e5136d8b97ea3d6f64c4aaa1215f4369832df432cab017396620bb6e30520a64180ceab6de222562ac11eab243a78dfa5a658ba018835a34caa19

@practicalswift

cf4b032 Use std::numeric_limits<UNSIGNED>::max()) instead of (UNSIGNED)-1 (practicalswift) 6b82fc5 Use const in COutPoint class (Hennadii Stepanov) Pull request description: 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). Tree-SHA512: fc7fe9838b6e5136d8b97ea3d6f64c4aaa1215f4369832df432cab017396620bb6e30520a64180ceab6de222562ac11eab243a78dfa5a658ba018835a34caa19

@practicalswift

cf4b032 Use std::numeric_limits<UNSIGNED>::max()) instead of (UNSIGNED)-1 (practicalswift) 6b82fc5 Use const in COutPoint class (Hennadii Stepanov) Pull request description: 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). Tree-SHA512: fc7fe9838b6e5136d8b97ea3d6f64c4aaa1215f4369832df432cab017396620bb6e30520a64180ceab6de222562ac11eab243a78dfa5a658ba018835a34caa19

@practicalswift

cf4b032 Use std::numeric_limits<UNSIGNED>::max()) instead of (UNSIGNED)-1 (practicalswift) 6b82fc5 Use const in COutPoint class (Hennadii Stepanov) Pull request description: 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). Tree-SHA512: fc7fe9838b6e5136d8b97ea3d6f64c4aaa1215f4369832df432cab017396620bb6e30520a64180ceab6de222562ac11eab243a78dfa5a658ba018835a34caa19

@practicalswift

cf4b032 Use std::numeric_limits<UNSIGNED>::max()) instead of (UNSIGNED)-1 (practicalswift) 6b82fc5 Use const in COutPoint class (Hennadii Stepanov) Pull request description: 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). Tree-SHA512: fc7fe9838b6e5136d8b97ea3d6f64c4aaa1215f4369832df432cab017396620bb6e30520a64180ceab6de222562ac11eab243a78dfa5a658ba018835a34caa19

@practicalswift

cf4b032 Use std::numeric_limits<UNSIGNED>::max()) instead of (UNSIGNED)-1 (practicalswift) 6b82fc5 Use const in COutPoint class (Hennadii Stepanov) Pull request description: 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). Tree-SHA512: fc7fe9838b6e5136d8b97ea3d6f64c4aaa1215f4369832df432cab017396620bb6e30520a64180ceab6de222562ac11eab243a78dfa5a658ba018835a34caa19

@practicalswift

cf4b032 Use std::numeric_limits<UNSIGNED>::max()) instead of (UNSIGNED)-1 (practicalswift) 6b82fc5 Use const in COutPoint class (Hennadii Stepanov) Pull request description: 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). Tree-SHA512: fc7fe9838b6e5136d8b97ea3d6f64c4aaa1215f4369832df432cab017396620bb6e30520a64180ceab6de222562ac11eab243a78dfa5a658ba018835a34caa19

@practicalswift

cf4b032 Use std::numeric_limits<UNSIGNED>::max()) instead of (UNSIGNED)-1 (practicalswift) 6b82fc5 Use const in COutPoint class (Hennadii Stepanov) Pull request description: 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). Tree-SHA512: fc7fe9838b6e5136d8b97ea3d6f64c4aaa1215f4369832df432cab017396620bb6e30520a64180ceab6de222562ac11eab243a78dfa5a658ba018835a34caa19

fanquake added the Refactoring label Nov 29, 2018

practicalswift reviewed Nov 29, 2018

View reviewed changes

src/primitives/transaction.h Outdated Show resolved Hide resolved

hebasto force-pushed the 20181129-const-null-outpoint branch from 95e2c1a to 6a53717 Compare November 29, 2018 09:53

Use const in COutPoint class

6b82fc5

hebasto force-pushed the 20181129-const-null-outpoint branch from 6a53717 to 6b82fc5 Compare November 30, 2018 10:54

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

cf4b032

Empact reviewed Dec 6, 2018

View reviewed changes

laanwj merged commit cf4b032 into bitcoin:master Dec 6, 2018

hebasto deleted the 20181129-const-null-outpoint branch December 6, 2018 14:48

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Use const in COutPoint class #14838

Use const in COutPoint class #14838

Uh oh!

Conversation

hebasto commented Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Nov 29, 2018

Uh oh!

Empact commented Nov 30, 2018

Uh oh!

hebasto commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Dec 3, 2018

Uh oh!

practicalswift commented Dec 3, 2018

Uh oh!

maflcko commented Dec 3, 2018

Uh oh!

Empact commented Dec 4, 2018

Uh oh!

practicalswift commented Dec 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Dec 4, 2018

Uh oh!

practicalswift commented Dec 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Dec 4, 2018

Uh oh!

practicalswift commented Dec 4, 2018

Uh oh!

Empact commented Dec 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Dec 5, 2018

Uh oh!

Empact commented Dec 5, 2018

Uh oh!

promag commented Dec 5, 2018

Uh oh!

Empact Dec 6, 2018

Choose a reason for hiding this comment

Uh oh!

Empact Dec 6, 2018

Choose a reason for hiding this comment

Uh oh!

Empact Dec 6, 2018

Choose a reason for hiding this comment

Uh oh!

Empact commented Dec 6, 2018

Uh oh!

laanwj commented Dec 6, 2018

Uh oh!

Uh oh!

hebasto commented Nov 29, 2018 •

edited

Loading

practicalswift commented Nov 29, 2018 •

edited

Loading

hebasto commented Nov 29, 2018 •

edited

Loading

hebasto commented Nov 30, 2018 •

edited

Loading

practicalswift commented Dec 4, 2018 •

edited

Loading

practicalswift commented Dec 4, 2018 •

edited

Loading

Empact commented Dec 4, 2018 •

edited

Loading