Skip to content
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

Merged
merged 2 commits into from
Jan 5, 2024

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Jan 4, 2024

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

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.
@iphydf iphydf added this to the v0.2.19 milestone Jan 4, 2024
@iphydf iphydf marked this pull request as ready for review January 4, 2024 13:31
Copy link

codecov bot commented Jan 4, 2024

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (cac074c) 68.96% compared to head (79f55bd) 68.98%.

Files Patch % Lines
toxcore/tox.c 73.91% 6 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@nurupo
Copy link
Member

nurupo commented Jan 4, 2024

Eh, an alias? That's somewhat useless, isn't it? One can still just pass uint32_t for it. I guess it does serve a documentive purpose... But why not just make them structs at that point, so that each are a unique type of their own, instead of just an alias? Could even be an opaque struct instead of a struct of uint32_t. The client code shouldn't rely on the values of those uint32_ts anyway. That would break both API and ABI though, so I guess that would be one reason not to do it.

@iphydf
Copy link
Member Author

iphydf commented Jan 4, 2024

Eh, an alias? That's somewhat useless, isn't it? One can still just pass uint32_t for it. I guess it does serve a documentive purpose... But why not just make them structs at that point, so that each are a unique type of their own, instead of just an alias? Could even be an opaque struct instead of a struct of uint32_t. The client code shouldn't rely on the values of those uint32_ts anyway. That would break both API and ABI though, so I guess that would be one reason not to do it.

The point is to show which uint32_t goes where. Sure, the client won't do anything to them, but they might pass a friend number where a peer number is required. This makes that a bit clearer from the function signature. Structs would break the API (not the ABI), so I don't want to do that in 0.2.x.

@nurupo
Copy link
Member

nurupo commented Jan 4, 2024

Structs would break the API (not the ABI)

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

Copy link
Member

@nurupo nurupo left a 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: :shipit: complete! 1 of 1 approvals obtained

@nurupo
Copy link
Member

nurupo commented Jan 4, 2024

CI is complaining that function declarations in tox.c weren't updated to use arrays, they still use pointers. Other than that - LGTM.

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.
@toktok-releaser toktok-releaser merged commit 79f55bd into TokTok:master Jan 5, 2024
53 checks passed
@iphydf iphydf deleted the typedefs branch January 5, 2024 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants