univalue: Avoid brittle, narrowing and verbose integral type confusions #25611

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2207-univalue-types-🙉 changing 3 files +24 −26

MarcoFalke commented at 9:06 AM on July 14, 2022: member

As UniValue provides several constructors for integral types, the compiler is unable to select one if the passed type does not exactly match. This is unintuitive for developers and forces them to write verbose and brittle code. (Refer to -Wnarrowing compiler warning)

For example, there are many places where an unsigned int is cast to a signed int. While the cast is safe in practice, it is still needlessly verbose and confusing as the value can never be negative. In fact it might even be unsafe if the unsigned value is large enough to map to a negative signed one.

Fix this issue and other (minor) type issues.
MarcoFalke added the label Refactoring on Jul 14, 2022

rpc: Select int-UniValue constructor for enum value in upgradewallet RPC

UniValue does not have a constructor for enum values, however the
compiler will decay the enum into an int and select that constructor.
Avoid this compiler magic and clarify the code by explicitly selecting
the int-constructor.

This is needed for the next commit.

fa3a9a1e8d

MarcoFalke force-pushed on Jul 14, 2022
MarcoFalke force-pushed on Jul 14, 2022

univalue: Avoid narrowing and verbose int constructors

As UniValue provides several constructors for integral types, the
compiler is unable to select one if the passed type does not exactly
match. This is unintuitive for developers and forces them to write
verbose and brittle code.

For example, there are many places where an unsigned int is cast to a
signed int. While the cast is safe in practice, it is still needlessly
verbose and confusing as the value can never be negative. In fact it
might even be unsafe if the unsigned value is large enough to map to a
negative signed one.

fa23c19750

MarcoFalke force-pushed on Jul 14, 2022
MarcoFalke force-pushed on Jul 14, 2022
MarcoFalke force-pushed on Jul 14, 2022
aureleoules commented at 2:39 PM on July 18, 2022: member

ACK fa23c197509f692a815193acc1b50bad2fcbedfe. I verified that this change prevents type narrowing when not casting explicitly.

nit: shouldn't the implementation be moved to univalue.cpp for consistency? https://github.com/bitcoin/bitcoin/blob/fa23c197509f692a815193acc1b50bad2fcbedfe/src/univalue/include/univalue.h#L55
MarcoFalke commented at 2:46 PM on July 18, 2022: member

nit: shouldn't the implementation be moved to univalue.cpp for consistency?

I think oneline stuff is fine to keep inlined. Also, seems unrelated to the changes here?
fanquake merged this on Jul 25, 2022
fanquake closed this on Jul 25, 2022
MarcoFalke deleted the branch on Jul 25, 2022
sidhujag referenced this in commit 3cccd7835f on Jul 25, 2022
bitcoin locked this on Jul 25, 2023

Contributors

MarcoFalke

aureleoules

Labels

Refactoring