univalue: Avoid brittle, narrowing and verbose integral type confusions#25611
Merged
fanquake merged 2 commits intobitcoin:masterfrom Jul 25, 2022
Hidden character warning
The head ref may contain hidden characters: "2207-univalue-types-\ud83d\ude49"
Merged
univalue: Avoid brittle, narrowing and verbose integral type confusions#25611fanquake merged 2 commits intobitcoin:masterfrom
fanquake merged 2 commits intobitcoin:masterfrom
Conversation
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.
faa2fbd to
fa83573
Compare
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.
fa94dee to
fa23c19
Compare
aureleoules
reviewed
Jul 18, 2022
Contributor
aureleoules
left a comment
There was a problem hiding this comment.
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?
bitcoin/src/univalue/include/univalue.h
Line 55 in fa23c19
Member
Author
I think oneline stuff is fine to keep inlined. Also, seems unrelated to the changes here? |
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
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.
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
-Wnarrowingcompiler 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.