p2p: declare Announcement::m_state as uint8_t, add getter/setter #20162
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
If you increase the size of m_state, please also decrease the size of m_sequence (54 bits is still plenty!), otherwise the memory usage of an Announcement object will increase by 16 bytes. An alternative is changing the type to uint8_t, and having a getter/setter on Announcement to convert/from to State. |
|
Needs to shrink The original solution was making |
|
@ajtowns Priority isn't stored anywhere, it's computed on the fly. But sequence can be shrunk with no downside really. |
fa1cf39 to
0f298a2
Compare
|
Thanks -- adjusted |
0bd2602 to
39cb2ba
Compare
|
Previous commit 0bd2602 added 5 bits to Current commit 39cb2ba follows your suggestions to do |
sipa
reviewed
Oct 16, 2020
to silence these Travis CI GCC compiler warnings:
txrequest.cpp:73:21: warning: ‘{anonymous}::Announcement::m_state’ is
too small to hold all values of ‘enum class {anonymous}::State’
State m_state : 3;
^
The warnings are based on the maximum value held by the underlying uint8_t
enumerator type, though the intention of the bitfield declaration is the
maximum declared enumerator value.
The warning been silenced in GCC 8.4+ and 9.3+ according to
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61414
39cb2ba to
c8abbc9
Compare
|
utACK c8abbc9 |
|
ACK c8abbc9 -- quick code review |
hebasto
approved these changes
Oct 19, 2020
There was a problem hiding this comment.
ACK c8abbc9, tested on Bionic (x86_64, gcc 7.5.0):
- master (4769942):
$ make clean && make > /dev/null
txrequest.cpp:73:21: warning: ‘{anonymous}::Announcement::m_state’ is too small to hold all values of ‘enum class {anonymous}::State’
State m_state : 3;
^
- this PR (c8abbc9):
$ make clean && make > /dev/null
# no warnings :)
I verified that the following statements did not changed:
sizeof(Announcement) == size_t(56)alignof(Announcement) == size_t(8)
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Oct 19, 2020
…dd getter/setter c8abbc9 p2p: declare Announcement::m_state as uint8_t, add getter/setter (Jon Atack) Pull request description: Change `Announcement::m_state` in `tx_request.cpp` from type `State` to `uint8_t` and add a getter and setter for the conversion to/from `State`. This should silence these travis ci gcc compiler warnings: ``` txrequest.cpp:73:21: warning: ‘{anonymous}::Announcement::m_state’ is too small to hold all values of ‘enum class {anonymous}::State’ State m_state : 3; ^ ``` The gcc warnings are based on the maximum value held by the underlying uint8_t enumerator type, even though the intention of the bitfield declaration is the maximum declared enumerator value. They have apparently been silenced in gcc 8.4+ and 9.3+ according to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61414. ACKs for top commit: sipa: utACK c8abbc9 ajtowns: ACK c8abbc9 -- quick code review hebasto: ACK c8abbc9, tested on Bionic (x86_64, gcc 7.5.0): Tree-SHA512: 026721dd7a78983a72da77638d3327d2b252bef804e489278a852f000046c028d6557bbd6c2b4cea391d4e01f9264a1be842d502047cb90b2997cc37bee59e61
maflcko
pushed a commit
to maflcko/bitcoin-core
that referenced
this pull request
Dec 4, 2020
…ocument exceptions 2f6fe4e ci: Build with --enable-werror by default, and document exceptions (Hennadii Stepanov) Pull request description: This PR prevents introducing of new compiler warnings in the master branch, e.g., bitcoin#19986, bitcoin#20162. ACKs for top commit: practicalswift: cr ACK 2f6fe4e: patch looks correct MarcoFalke: re-ACK 2f6fe4e 🏏 vasild: ACK 2f6fe4e Tree-SHA512: 23b5feb5bc472658c992d882ef61af23496f25adaa19f9c79bfaef5d2db273d44981aa93b1631a7d37cb58755283c1dacf3f2d68e501522d3fa8c965ab646d19
deadalnix
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
May 20, 2021
Summary: ``` This adds a new module (unused for now) which defines TxRequestTracker, a data structure that maintains all information about transaction requests, and coordinates requests. ``` Partial backport of [[bitcoin/bitcoin#19988 | core#19988]]: bitcoin/bitcoin@da3b8fd#diff-e3e574cb6dabe01b6149f9d121fc7e286abb49c04442a0a816977ef2a4103ed8 This also includes a GCC workaround that would cause the build to fail: Partial backport of [[bitcoin/bitcoin#20162 | core#20162]]. This backport will be completed as PR19988 is ported because it impacts the code from other commits. Since this is a lot of changes the tests are done in separate diffs (as per the PR commits). For now the code is not used so this is safe. Test Plan: ninja Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9547
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
May 20, 2021
Summary: ``` Add unit tests for TxRequestTracker. Several scenarios are tested, randomly interleaved with eachother. Includes a test by Antoine Riard (ariard). ``` Partial backport of [[bitcoin/bitcoin#19988 | core#19988]]: bitcoin/bitcoin@3c7fe0e Partial backport of [[bitcoin/bitcoin#20162 | core#20162]]. Depends on D9547. Test Plan: ninja check Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: PiRK Differential Revision: https://reviews.bitcoinabc.org/D9548
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change
Announcement::m_stateintx_request.cppfrom typeStatetouint8_tand add a getter and setter for the conversion to/fromState. This should silence these travis ci gcc compiler warnings:The gcc warnings are based on the maximum value held by the underlying uint8_t enumerator type, even though the intention of the bitfield declaration is the maximum declared enumerator value. They have apparently been silenced in gcc 8.4+ and 9.3+ according to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61414.