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

WIP: Introduce (conditional) C23 fixed underlying types to enums #1417

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

seanm
Copy link
Contributor

@seanm seanm commented Dec 31, 2023

I took a stab at specifying some of the sizes, based on how they are used, but it's all provisional, and there may be backwards compatibility concerns...

@mcuee mcuee added the core Related to common codes label Jan 1, 2024
@Youw
Copy link
Member

Youw commented Jan 21, 2024

This will break the ABI backward compatibility.
It gets even worse, when the library is built with C23, installed into a system/etc., and then some other application is built with C11 with the same header/binary - those will be completely incompatible.

I don't think libusb can afford this change at all... At most - this can be optional config to be enabled at build time explicitly (and disabled by default) to be used by those who really need it explicitly in ther projects/application.

But I can't imagine a scenario what that is required by any of the sane use-cases.

@Youw Youw added the DNM! DO NOT MERGE! label Jan 21, 2024
@seanm
Copy link
Contributor Author

seanm commented Jan 21, 2024

This will break the ABI backward compatibility.

Only if the size/sign/type is actually changed (as opposed to merely being stated explicitly).

And for private enums, we can change the ABI/API, can we not?

For public enums, then yes, I agree we need to be very careful about changing them

@Youw
Copy link
Member

Youw commented Jan 21, 2024

And for private enums, we can change the ABI/API, can we not?

Yes, that shold not harm.

Only if the size/sign/type is actually changed (as opposed to merely being stated explicitly).

Unless stated explicitly (in C23), the enums in C are backed by the int. (Cannot find a reference to the standard but at least one compiler with enabled optimisations confirms it: https://godbolt.org/z/ndjxzaM5h).

And as far as this PR suggests - many public-declared enums are declared to be other (smaller) than int.
Even explicitly specifying uint32_t is unsafe - on some systems int (so are enums) is 64bit.

To be honest I don't really get the point of explicilty specifying the enum type to be int explicitly for any enum. It is just redundant verbosity to my taste.

@Youw
Copy link
Member

Youw commented Jan 21, 2024

@tormodvolden do you happen to have an oppinion on this one?

@seanm
Copy link
Contributor Author

seanm commented Jan 21, 2024

Unless stated explicitly (in C23), the enums in C are backed by the int. (Cannot find a reference to the standard but at least one compiler with enabled optimisations confirms it: https://godbolt.org/z/ndjxzaM5h).

Here's some details: https://en.cppreference.com/w/c/language/enum

To be honest I don't really get the point of explicilty specifying the enum type to be int explicitly for any enum. It is just redundant verbosity to my taste.

It's quite helpful when converting between an enum and a raw integer type (which libusb does a lot). It helps the compiler warn about signed/unsigned mismatches, and size mismatches.

For example, enum libusb_descriptor_type where I added LIBUSB_ENUM_SIZE(uint8_t). It's important that the values in this enum never be bigger (or negative) because they are stored into structs like this:

struct libusb_device_descriptor {
	...
	uint8_t  bDescriptorType;
	...

By tagging the enum in this way it 1) acts like documentation 2) the compiler can ensure values do not exceed what's expected.

I agree there are compatibility concerns. But for me, opting into a new language version (with -std=c23), is a good opportunity for a small compatibility break.

@Youw
Copy link
Member

Youw commented Jan 22, 2024

But for me, opting into a new language version (with -std=c23), is a good opportunity for a small compatibility break.

That's a huge compatibility break :)

To have it as an improvement - it has to be a build/configuration option that has to be explicitly enabled, rather than depend on the compiler capabilities.
That way libusb developers/contributors/CI can leverage these extra compiler checks, and user won't get their builds accidentally broken: imagine a case where a new version of libusb is released into some distro, and the header would have this "if C23" check. The distro would likey build with C11 at most, but some user may want to try to use C23 locally for their project.
The runtime would crash, and the user wouldn't have a clue what's going on.

I took a stab at specifying some of the sizes, based on how they are used, but it's all provisional, and there may be backwards compatibility concerns...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Related to common codes DNM! DO NOT MERGE!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants