-
Notifications
You must be signed in to change notification settings - Fork 291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cleanup: Add typedefs for public API int identifiers. #2518
Conversation
These make it clearer which kinds of `uint32_t` can fit in which functions. It also makes it easier to generate higher level bindings without doing a lot of inference/heuristics.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2518 +/- ##
==========================================
+ Coverage 68.96% 68.98% +0.01%
==========================================
Files 89 89
Lines 27781 27781
==========================================
+ Hits 19160 19165 +5
+ Misses 8621 8616 -5 ☔ View full report in Codecov by Sentry. |
Eh, an alias? That's somewhat useless, isn't it? One can still just pass |
The point is to show which |
Correct, non-opaque structs wouldn't break the ABI. A struct of uint32_t should have the same ABI as just uint32_t. However, opaque structs -- a pointer to a struct, e.g. for Friend Number (would have to be renamed to Friend Handle since it's no longer a number?) would break the ABI. 32-bit uint32_t vs 64-bit/platform-dependent-size pointer to a struct. But you already know all that better than I do :P |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big improvement.
Personally I would prefer to go even further with this and use distinct types, e.g. structs and even opaque structs in cases where uint32_t is an internal implementation detail that user isn't supposed to use for anything, but this non-breaking change is fine too. Maybe sometime in the future.
CI is complaining that function declarations in tox.c weren't updated to use arrays, they still use pointers. Other than that - LGTM.
Reviewed 1 of 1 files at r2, 1 of 1 files at r4.
Reviewable status:complete! 1 of 1 approvals obtained
Ah, looks like that was already fixed while I was away. |
These are ignored by C compilers, but can be used as documentation and by bindings generators.
These make it clearer which kinds of
uint32_t
can fit in which functions. It also makes it easier to generate higher level bindings without doing a lot of inference/heuristics.This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)