p2p: declare Announcement::m_state as uint8_t, add getter/setter #20162

1. 
   jonatack commented at 11:34 pm on October 15, 2020: member

   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:

   0txrequest.cpp:73:21: warning: ‘{anonymous}::Announcement::m_state’ is
   1too small to hold all values of ‘enum class {anonymous}::State’
   2     State m_state : 3;
   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.

2. 
   sipa commented at 11:38 pm on October 15, 2020: member

   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.

3. 
   ajtowns commented at 11:43 pm on October 15, 2020: member

   Needs to shrink m_sequence by 5 bits to keep the overall data structure size the same.

   The original solution was making m_state be a uint8_t : 3 and having a getter and setter that did the conversion to/from State rather than accessing it directly. I think that’s better than shrinking the sequence by 5 bits, but don’t have an opinion between doing the getter/setter and spurious warning in older versions of gcc. (EDIT: sequence not priority, duh)

4. 
   sipa commented at 11:45 pm on October 15, 2020: member

   @ajtowns Priority isn’t stored anywhere, it’s computed on the fly. But sequence can be shrunk with no downside really.
5. jonatack force-pushed on Oct 15, 2020
6. 
   jonatack commented at 11:51 pm on October 15, 2020: member

   Thanks – adjusted m_sequence accordingly. It’s late so will re-look at this tomorrow.
7. jonatack force-pushed on Oct 15, 2020
8. jonatack force-pushed on Oct 16, 2020
9. 
   jonatack commented at 9:06 pm on October 16, 2020: member

   Previous commit 0bd2602 added 5 bits to m_state and removed 5 from m_sequence.

   Current commit 39cb2ba follows your suggestions to do uint8_t m_state : 3 with a getter and setter.

10. p2p: declare Announcement::m_state as uint8_t, add getter/setter

    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

    c8abbc9d1f
11. in src/txrequest.cpp:73 in 39cb2ba486 outdated

    69@@ -70,31 +70,38 @@ struct Announcement {
    70     const bool m_is_wtxid : 1;
    71 
    72     /** What state this announcement is in. */
    73-    State m_state : 3;
    74+    /** This is a uint8_t instead of a State to silence a GCC warning. */
    

    ---

    ---

    

    sipa commented at 9:12 pm on October 16, 2020:

    Perhaps document which GCC version(s) and a link to the bug, so that we can decide in the future if it’s worth maintaining this workaround.

    (I initially had this approach in my PR, but removed it after I couldn’t reproduce it anymore).

    ---

    

    jonatack commented at 9:17 pm on October 16, 2020:

    Good idea. Yes, this is the approach in your commit daa4f59b, the first one I reviewed.

    ---

    

    jonatack commented at 9:36 pm on October 16, 2020:

    Done
12. jonatack force-pushed on Oct 16, 2020
13. jonatack renamed this:
    p2p, compiler warnings: specify Announcement::m_state bitfield to be 8 bits
    p2p: declare Announcement::m_state as uint8_t, add getter/setter
    on Oct 16, 2020
14. 
    sipa commented at 10:30 pm on October 16, 2020: member

    utACK c8abbc9d1fd8f77b5dcf31cf7a6dd6aac5d1c75c
15. DrahtBot added the label P2P on Oct 16, 2020
16. 
    ajtowns commented at 1:37 am on October 17, 2020: member

    ACK c8abbc9d1fd8f77b5dcf31cf7a6dd6aac5d1c75c – quick code review
17. hebasto approved
18. 
    hebasto commented at 11:03 am on October 19, 2020: member

    ACK c8abbc9d1fd8f77b5dcf31cf7a6dd6aac5d1c75c, tested on Bionic (x86_64, gcc 7.5.0):

    • master (4769942d901a6095dc8715b6008d608e62d10b3c):

    0$ make clean && make > /dev/null
    1txrequest.cpp:73:21: warning: ‘{anonymous}::Announcement::m_state’ is too small to hold all values of ‘enum class {anonymous}::State’
    2     State m_state : 3;
    3                     ^
    

    • this PR (c8abbc9d1fd8f77b5dcf31cf7a6dd6aac5d1c75c):

    0$ make clean && make > /dev/null
    1# no warnings :)
    

    I verified that the following statements did not changed:

    • sizeof(Announcement) == size_t(56)
    • alignof(Announcement) == size_t(8)
19. MarcoFalke added the label Refactoring on Oct 19, 2020
20. MarcoFalke merged this on Oct 19, 2020
21. MarcoFalke closed this on Oct 19, 2020

    ---

22. jonatack deleted the branch on Oct 19, 2020
23. sidhujag referenced this in commit 199be16fdd on Oct 19, 2020
24. MarcoFalke referenced this in commit e2ae6a2bef on Dec 4, 2020
25. deadalnix referenced this in commit 07731aaf05 on May 20, 2021
26. Fabcien referenced this in commit 47c2219049 on May 20, 2021
27. DrahtBot locked this on Feb 15, 2022

Contributors
jonatack sipa ajtowns hebasto

Labels
Refactoring P2P