Skip to content

Conversation

ryanofsky
Copy link
Contributor

Switches to named constants, because numeric_limits calls can be harder to read and less portable.

Change was suggested by jamesob in #10973 (comment)

There are no changes in behavior except on some platforms we don't support (ILP64, IP16L32, I16LP32), where SignalsOptInRBF and MutateTxAddInput functions would now work correctly.

hebasto and others added 3 commits November 1, 2018 19:55
The deleted LOC is a dup so far as
`std::numeric_limits<unsigned int>::min()` == 0
Switches to named constants, because numeric_limits calls can be harder to read
and less portable.

Change was suggested by James O'Beirne <james.obeirne@gmail.com> in
bitcoin#10973 (comment)

There are no changes in behavior except on some platforms we don't support
(ILP64, IP16L32, I16LP32), where SignalsOptInRBF() and MutateTxAddInput()
functions would now work correctly.
@maflcko
Copy link
Member

maflcko commented Nov 1, 2018

Tagged "refactoring" because the platforms that are broken are supposedly not supported.

@practicalswift
Copy link
Contributor

utACK 8041cd3

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 1, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@fanquake
Copy link
Member

fanquake commented Nov 2, 2018

@ryanofsky Can you combine the test change from #14463 into this? I'll close that PR, as this has more extensive changes.

@ryanofsky
Copy link
Contributor Author

Updated 8041cd3 -> 90a9e72 (pr/climit.1 -> pr/climit.2) adding commits from #14463

@ryanofsky
Copy link
Contributor Author

Updated 90a9e72 -> 5352030 (pr/climit.2 -> pr/climit.3) adding U suffix (doesn't hurt to be explicit).

@practicalswift
Copy link
Contributor

practicalswift commented Nov 5, 2018

@ryanofsky I think the U suffix suggestion was based on a misunderstanding regarding the types of C++ integer literals.

More specifically that the integer literal of 4294967295 can be assumed to have the same type as the integer literal 0xffffffff.

That assumption is false :-)

Proof:

[cling]$ #include <typeindex>
[cling]$ #include <typeinfo>
[cling]$ std::type_index(typeid(4294967295)) == std::type_index(typeid(0xffffffff))
(bool) false
[cling]$ 4294967295
(long) 4294967295
[cling]$ 0xffffffff
(unsigned int) 4294967295

@ryanofsky
Copy link
Contributor Author

The u suffix isn't needed, but it's nice because it makes it more obvious that compiler will choose an unsigned type. (According to the "type of the literal" table in https://en.cppreference.com/w/cpp/language/integer_literal, the compiler without the u would choose smallest signed or unsigned type that holds the value.)

@jimpo
Copy link
Contributor

jimpo commented Nov 6, 2018

ACK 5352030

@maflcko
Copy link
Member

maflcko commented Nov 6, 2018

utACK 5352030

  • Checked that e4dc39b happens to produce the same bitcoind binaries on my arch
  • Don't care about bafb921
  • Checked that 5352030 happens to produce the same bitcoin-tx and bitcoind binaries on my arch.

@maflcko maflcko added this to the 0.18.0 milestone Nov 6, 2018
@ken2812221
Copy link
Contributor

utACK 5352030

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Nov 7, 2018
…and lock times

5352030 Avoid using numeric_limits for sequence numbers and lock times (Russell Yanofsky)
bafb921 Remove duplicated code (Hennadii Stepanov)
e4dc39b Replace platform dependent type with proper const (Hennadii Stepanov)

Pull request description:

  Switches to named constants, because numeric_limits calls can be harder to read and less portable.

  Change was suggested by jamesob in bitcoin#10973 (comment)

  There are no changes in behavior except on some platforms we don't support (ILP64, IP16L32, I16LP32), where `SignalsOptInRBF` and `MutateTxAddInput` functions would now work correctly.

Tree-SHA512: 3f5c6393c260551f65a0edfba55ef7eb3625232eec8d85b1457f26e144aa0b90c7ef5f44b2fd2f7d9be3c3bcb301030a9f5473c21b3bac566cc59b8c8780737c
@maflcko maflcko merged commit 5352030 into bitcoin:master Nov 7, 2018
5tefan added a commit to 5tefan/dash that referenced this pull request Jul 28, 2021
Merges bitcoin#14636: Avoid using numeric_limits for sequence
numbers and lock times.

5352030 Avoid using numeric_limits for sequence numbers and lock
            times (Russell Yanofsky)
bafb921 Remove duplicated code (Hennadii Stepanov)
e4dc39b Replace platform dependent type with proper const
            (Hennadii Stepanov)

Pull request description:

  Switches to named constants, because numeric_limits calls can be
harder to read and less portable.

  Change was suggested by jamesob in
bitcoin#10973 (comment)

  There are no changes in behavior except on some platforms we don't
support (ILP64, IP16L32, I16LP32), where `SignalsOptInRBF` and
`MutateTxAddInput` functions would now work correctly.
5tefan added a commit to 5tefan/dash that referenced this pull request Jul 28, 2021
Merges bitcoin#14636: Avoid using numeric_limits for sequence
numbers and lock times.

5352030 Avoid using numeric_limits for sequence numbers and lock
            times (Russell Yanofsky)
bafb921 Remove duplicated code (Hennadii Stepanov)
e4dc39b Replace platform dependent type with proper const
            (Hennadii Stepanov)

Pull request description:

  Switches to named constants, because numeric_limits calls can be
harder to read and less portable.

  Change was suggested by jamesob in
bitcoin#10973 (comment)

  There are no changes in behavior except on some platforms we don't
support (ILP64, IP16L32, I16LP32), where `SignalsOptInRBF` and
`MutateTxAddInput` functions would now work correctly.
5tefan added a commit to 5tefan/dash that referenced this pull request Jul 28, 2021
Merges bitcoin#14636: Avoid using numeric_limits for sequence
numbers and lock times.

5352030 Avoid using numeric_limits for sequence numbers and lock
            times (Russell Yanofsky)
bafb921 Remove duplicated code (Hennadii Stepanov)
e4dc39b Replace platform dependent type with proper const
            (Hennadii Stepanov)

Pull request description:

  Switches to named constants, because numeric_limits calls can be
harder to read and less portable.

  Change was suggested by jamesob in
bitcoin#10973 (comment)

  There are no changes in behavior except on some platforms we don't
support (ILP64, IP16L32, I16LP32), where `SignalsOptInRBF` and
`MutateTxAddInput` functions would now work correctly.
PastaPastaPasta pushed a commit to dashpay/dash that referenced this pull request Jul 28, 2021
…and lock times (#4296)

Merges bitcoin#14636: Avoid using numeric_limits for sequence
numbers and lock times.

5352030 Avoid using numeric_limits for sequence numbers and lock
            times (Russell Yanofsky)
bafb921 Remove duplicated code (Hennadii Stepanov)
e4dc39b Replace platform dependent type with proper const
            (Hennadii Stepanov)

Pull request description:

  Switches to named constants, because numeric_limits calls can be
harder to read and less portable.

  Change was suggested by jamesob in
bitcoin#10973 (comment)

  There are no changes in behavior except on some platforms we don't
support (ILP64, IP16L32, I16LP32), where `SignalsOptInRBF` and
`MutateTxAddInput` functions would now work correctly.
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants