chore: replace MSVC-style long/long long with fixed-width integer types - #37
Open
TrueBrain wants to merge 1 commit into
Open
chore: replace MSVC-style long/long long with fixed-width integer types#37TrueBrain wants to merge 1 commit into
TrueBrain wants to merge 1 commit into
Conversation
MSVC's long is 32-bit and long long is 64-bit, but long is 64-bit on LP64 platforms (Linux/macOS). Use uint32_t/int32_t/uint64_t/int64_t instead so widths are explicit and consistent across compilers.
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 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.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
MSVC's long is 32-bit and long long is 64-bit, but long is 64-bit on LP64 platforms (Linux/macOS). Use uint32_t/int32_t/uint64_t/int64_t instead so widths are explicit and consistent across compilers.
I created this PR maybe more to start a conversation than to actually expect this to be accepted as-is. But when porting carbon to Linux, I ran into this in basically every repo.
The C++ standard only defines the minimum size of
longbeing 32bit. And for some fun historical reasons, MSVC was like: "sure, so 32bit, right?". And GCC was like: "no, let's do 64bit!". And a big problem was born.Now this problem doesn't surface as much when you primary work in MSVC: on Linux/MacOS it is just a bigger variable. The other way around however ....... it is much more problematic.
So generally what I tend to do and advise open source projects: as quickly as you can convert away from
long, and usecstdinttypes (uint32_t,int64_t, ...) instead. This is unambiguous, and avoids "works on my machine" (and those are some nightmare debug sessions .....).Now there are already
uint64_tusages, but in some repos it is a mix. Inbluefor example there is a definition forUnpackwithuint64_t[1], but the implementation is forunsigned long long[2]. It is a bit confusing :D(to make matters slightly worse, on Linux,
unsigned long longanduint64_tare not considered the same type, despite them resolving to the same byte-size. Well, I guess you get my point by now: mixing these types is painful when not on MSVC).Although
longis the only real problem child:short,unsigned char, etc are other examples of poorly defined "minimum size" typing. It sometimes can be good to just "rip the bandaid", and convert them all tocstdinttypes. The additional benefit is that in that case you can enable a clang-tidy validator to ensure nobody adds them new again.I have to admit: it always takes a bit of time to get used to no longer doing
short a, but honestly: long term, it is 100% worth it, and makes for so much more readable/understandable code. Which means easier for other people to contribute, etc.As example and further reading, Google also has it as part of their Style Guide: https://google.github.io/styleguide/cppguide.html#Integer_Types .
Anyway, these kind of changes are "patch killers", as they basically touch all the files, and any existing PR would require resolving after merging this. So some kind of planning and intention is warrant. If you decide to convert
short/unsigned chartoo, it might be better to do that in one PR, instead of multiple. More than happy to update this PR to include the other types, and also not a problem to apply this to every repo of carbonengine. But let's first have the dialog whether this is wanted in the first place :)Appreciate your time!
Full disclosure: although I work for Fenris Creations, I have no involvement with the Carbon project. I work on this in my free time under my own name.