refactor: Make MessageBoxFlags enum underlying type unsigned #24191

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2201-nouiInt changing 2 files +1 −3

MarcoFalke commented at 11:13 AM on January 28, 2022: member

All values in the enum are unsigned. Also, flags shouldn't be treated as signed types. So clarify the underlying type and remove a sanitizer suppression.
DrahtBot added the label Refactoring on Jan 28, 2022

in src/node/ui_interface.h:28 in fa7f545149 outdated

  24 | @@ -25,8 +25,7 @@ class CClientUIInterface
  25 |  {
  26 |  public:
  27 |      /** Flags for CClientUIInterface::ThreadSafeMessageBox */
  28 | -    enum MessageBoxFlags
  29 | -    {
  30 | +    enum MessageBoxFlags : unsigned {

luke-jr commented at 3:40 AM on January 31, 2022:

unsigned is technically only guaranteed to be 16-bit, but StandardButton has values approaching 32-bit, so IMO better to specify unsigned long or uint32_t here.

MarcoFalke commented at 7:17 AM on January 31, 2022:

Bitcoin Core can't start nor compile with 16-bit int. See also:

src/compat/assumptions.h:static_assert(sizeof(short) == 2, "16-bit short assumed");
src/compat/assumptions.h:static_assert(sizeof(int) == 4, "32-bit int assumed");
src/compat/assumptions.h:static_assert(sizeof(unsigned) == 4, "32-bit unsigned assumed");
src/compat/assumptions.h:static_assert(sizeof(size_t) == 4 || sizeof(size_t) == 8, "size_t assumed to be 32-bit or 64-bit");

luke-jr commented at 8:08 AM on January 31, 2022:

Yes, but that's not a reason to make it worse when it could easily be made well-defined.

MarcoFalke commented at 8:28 AM on January 31, 2022:

Done, switched to uint32_t.

luke-jr changes_requested
refactor: Make MessageBoxFlags enum underlying type unsigned 1111d33532
MarcoFalke force-pushed on Jan 31, 2022
MarcoFalke commented at 8:38 AM on January 31, 2022: member

I checked on my system that bitcoind is not affected by this code change. Same binary with gcc and clang on O2.
hebasto approved
hebasto commented at 8:42 AM on January 31, 2022: member

ACK 1111d33532516c16fb2e22660ac2745ce56ad6cd, I have reviewed the code and it looks OK, I agree it can be merged.
luke-jr approved
luke-jr commented at 9:07 AM on January 31, 2022: member

utACK
MarcoFalke merged this on Jan 31, 2022
MarcoFalke closed this on Jan 31, 2022
MarcoFalke deleted the branch on Jan 31, 2022
sidhujag referenced this in commit 421e9fc0eb on Feb 1, 2022
Fabcien referenced this in commit 54363e9dbc on Dec 9, 2022
DrahtBot locked this on Jan 31, 2023

Contributors

Labels