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

libhackrf: use stdint integer types #1345

Closed

Conversation

Sasszem
Copy link
Contributor

@Sasszem Sasszem commented Jul 5, 2023

I went through libhackrf/hackrf.h and libhackrf/hackrf.c and replaced 'normal' integer types with stdint.h int types to make them specific in size.
This should not break any compatibility, as it's only a name change.
I left string types unchanged as char* since they should be handled as strings, not as integer arrays.

left a few char* types, because they should always be handled as strings
@martinling
Copy link
Member

It might arguably have been better to use explicitly-sized types in the API from the start, but now that the library is in use we can't change the types retroactively.

In C the choice of which types have which sizes is up to the platform and the compiler, so although e.g. an int and an int32_t might be the same thing on some platforms, that will not necessarily be the case everywhere. Substituting one for the other would be a backwards-incompatible change on platforms where the types don't match.

In practice it's possible that all the types you've substituted are equivalent on all the platforms that libhackrf can be built for in practice. I'm not sure whether that's actually true, but it could be. But if it is, then although this change would turn out to be safe, it's also unnecessary: because if the platforms all agree on the size of the generic types, specifying the sizes doesn't actually add any information for anyone.

So either way, this isn't a good idea, and we're not going to merge this change.

@martinling martinling closed this Nov 17, 2023
@Sasszem Sasszem deleted the libhackrf_integer_types branch February 23, 2024 11:50
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.

2 participants