Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upCore/Movie: Use fmt where applicable #8180
Merged
Conversation
While we do have this library as part of the public linkage interface in the common library target, which will be used in the future for the logging macros, we should still be explicit that we're using this library. Therefore, we privately link it in to be explicit about it.
In a few cases we needed to alter... less than ideal parameter types. While u8 may have been OK with printf-style formatting, which promotes most smaller types back to int, this won't work with fmt. fmt preserves the type of the passed in arguments, meaning that u8, being an alias of uint8_t (itself being an alias of unsigned char on all the platforms we support), will print out as a character, not a numeric value. As such, we amend some functions to operate on u32 values for two reasons: 1. We actually want it to print out as a value 2. Arithmetic on unsigned types smaller than unsigned int will actually promote to an int, not unsigned int. This is very non-obvious to some and makes for error-prone code. < sizeof(int) types are great for storage, not so much for performing unsigned arithmetic, despite the signedness of the type.
This comment has been minimized.
This comment has been minimized.
Hmm, doesn't fmt explicitly print unsigned char and uint8_t as a number, and not a character? fmtlib/fmt#217 |
This comment has been minimized.
This comment has been minimized.
Ah good, I didn't know that was fixed. That'll make future changes less concerning :) I'll keep the change as is though, since we're still implicitly truncating from |
This comment has been minimized.
This comment has been minimized.
Warepire
commented
Jun 15, 2019
•
Additionally at least on some arches unnecessarily restricting memory usage will generate more complex assembly to access those variables, increasing the size of the binary. ARM64 is one example. (Just one more reason to get rid of pointless u8's) |
4edf174
into
dolphin-emu:master
10 checks passed
10 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
lioncash commentedJun 14, 2019
This is split into its own PR, as it makes mild changes to some of the internal functions in Movie.cpp (namely changing
u8
parameters tou32
), and not purely replacingStringFromFormat
calls.While
u8
may have been OK with printf-style formatting, which promotes most smaller types back toint
internally, this won't work with fmt. fmt preserves the type of the passed in arguments, meaning thatu8
, being an alias ofuint8_t
(itself being an alias ofunsigned char
on all the platforms we support), will print out as a character, not a numeric value.Because of that, we amend some functions to operate on
u32
values for two reasons:We actually want said parameters to print out as a value, not a character, and casts are gross.
Arithmetic on unsigned types smaller than
unsigned int
will actually promote to anint
, notunsigned int
. This is very non-obvious to some and makes for error-prone code.< sizeof(int)
types are great for storage, not so much for performing unsigned arithmetic, despite the unsignedness of the type.