Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Jul 14, 2022

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.

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.
@maflcko maflcko force-pushed the 2207-univalue-types- branch 2 times, most recently from faa2fbd to fa83573 Compare July 14, 2022 10:00
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.
@maflcko maflcko force-pushed the 2207-univalue-types- branch 3 times, most recently from fa94dee to fa23c19 Compare July 14, 2022 11:48
Copy link
Contributor

@aureleoules aureleoules left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

bool setInt(int val_) { return setInt((int64_t)val_); }

@maflcko
Copy link
Member Author

maflcko commented Jul 18, 2022

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 fanquake merged commit 73a0d6d into bitcoin:master Jul 25, 2022
@maflcko maflcko deleted the 2207-univalue-types-🙉 branch July 25, 2022 14:24
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 25, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Jul 25, 2023
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.

3 participants