Avoid --Wconversion warnings #1497
Conversation
|
The last commit shows the alternative of using macros instead of an enum. The same could be done with the itransfer->state_flags like USBI_TRANSFER_IN_FLIGHT. |
|
I started on linux_usbfs, but there is a lot more to do. |
| enum usbi_event_flags { | ||
| /* The list of event sources has been modified */ | ||
| USBI_EVENT_EVENT_SOURCES_MODIFIED = 1U << 0, | ||
| /* event flags of the ctx structure */ | ||
| /* The list of event sources has been modified */ | ||
| #define USBI_EVENT_EVENT_SOURCES_MODIFIED (1U << 0) | ||
|
|
||
| /* The user has interrupted the event handler */ | ||
| USBI_EVENT_USER_INTERRUPT = 1U << 1, | ||
| /* The user has interrupted the event handler */ | ||
| #define USBI_EVENT_USER_INTERRUPT (1U << 1) | ||
|
|
||
| /* A hotplug callback deregistration is pending */ | ||
| USBI_EVENT_HOTPLUG_CB_DEREGISTERED = 1U << 2, | ||
| /* A hotplug callback deregistration is pending */ | ||
| #define USBI_EVENT_HOTPLUG_CB_DEREGISTERED (1U << 2) | ||
|
|
||
| /* One or more hotplug messages are pending */ | ||
| USBI_EVENT_HOTPLUG_MSG_PENDING = 1U << 3, | ||
| /* One or more hotplug messages are pending */ | ||
| #define USBI_EVENT_HOTPLUG_MSG_PENDING (1U << 3) | ||
|
|
||
| /* One or more completed transfers are pending */ | ||
| USBI_EVENT_TRANSFER_COMPLETED = 1U << 4, | ||
| /* One or more completed transfers are pending */ | ||
| #define USBI_EVENT_TRANSFER_COMPLETED (1U << 4) | ||
|
|
||
| /* A device is in the process of being closed */ | ||
| USBI_EVENT_DEVICE_CLOSE = 1U << 5, | ||
| }; |
There was a problem hiding this comment.
This seems a backwards change. enums should be encouraged over #defines, not the other way around.
Better would be to do this: 6b52b8e
There was a problem hiding this comment.
Hmm, I don't think we should bump compiler requirements for something so cosmetic. I understand that the suggestion doesn't strictly require C23 but it wouldn't be tested without.
There are certainly reasons for encouraging enums (especially if they had types) over defines in many cases but I am not sure it applies to us here.
As I said initially, I have mixed feelings about all this because we try everything to please compilers but likely make the code less readable. Maybe the best way is just to keep -Wconversion out :)
There was a problem hiding this comment.
Hmm, I don't think we should bump compiler requirements for something so cosmetic.
To be clear, I'm not advocating bumping the minimum compiler requirements. As you see in 6b52b8e, the feature can be wrapped in a macro like we do already for LIBUSB_DEPRECATED_FOR.
I understand that the suggestion doesn't strictly require C23 but it wouldn't be tested without.
Well sure, but adding a C23 variant to CI shouldn't be too hard.
There are certainly reasons for encouraging enums (especially if they had types) over defines in many cases but I am not sure it applies to us here.
I have found it very helpful as documenation, especially where those enumerated values are converted in/out of raw uintX_t struct fields.
As I said initially, I have mixed feelings about all this because we try everything to please compilers but likely make the code less readable.
I find this change more readable because now I, the reader, know the size and range of the thing:
-enum libusb_endpoint_direction {
+enum libusb_endpoint_direction LIBUSB_ENUM_SIZE(uint8_t) {Maybe the best way is just to keep -Wconversion out :)
Indeed I have found -Wconversion something that can find problems, but is often too noisy. I haven't reviewed all this PR yet, but will...
|
The changes to |
…bility_descriptor() Signed-off-by: Tormod Volden <debian.tormod@gmail.com>
Signed-off-by: Tormod Volden <debian.tormod@gmail.com>
Signed-off-by: Tormod Volden <debian.tormod@gmail.com>
Signed-off-by: Tormod Volden <debian.tormod@gmail.com>
Signed-off-by: Tormod Volden <debian.tormod@gmail.com>
Signed-off-by: Tormod Volden <debian.tormod@gmail.com>
Add some explicit casts, and avoid some dubious recycling of the same helper variable for completely different purposes. Signed-off-by: Tormod Volden <debian.tormod@gmail.com>
Signed-off-by: Tormod Volden <debian.tormod@gmail.com>
Whereas the URB status field is signed int, the Linux USBDEVFS API declares the status field of the iso packet descriptor as unsigned int. We compare them with signed values in the switch cases, because the error values are negative (system error macros like ENOENT are positive). https://docs.kernel.org/driver-api/usb/usb.html https://docs.kernel.org/driver-api/usb/error-codes.html This avoids a -Wsign-conversion warning. Signed-off-by: Tormod Volden <debian.tormod@gmail.com>
Signed-off-by: Tormod Volden <debian.tormod@gmail.com>
Signed-off-by: Tormod Volden <debian.tormod@gmail.com>
Signed-off-by: Tormod Volden <debian.tormod@gmail.com>
The warning would appear when building for Windows. Signed-off-by: Tormod Volden <debian.tormod@gmail.com>
The second argument to open() is an int carrying flags (including "access modes"). A third, optional argument has type mode_t, carrying "file mode bits" for file permissions. Also rename the variable to access_mode to make it more clear. The type mismatch was caught by building with -Wconversion. References #1497 Signed-off-by: Tormod Volden <debian.tormod@gmail.com>
The type mismatch was caught by building with -Wconversion. References #1497 Signed-off-by: Tormod Volden <debian.tormod@gmail.com>
tormodvolden
force-pushed
the
conversion_warnings
branch
from
cc8455d
to
7a206bd
Compare
|
I merged the two actual fixes for linux. For the rest I am in no hurry. It was more an experiment to see what would be needed to become -Wconversion clean, it become quickly an obsession. -Wconversion demonstratively has a value, as it found the two Linux errors, but is all of it worth all the clutter? |
This allows building without warnings even with -Wconversion added to CFLAGS. At the moment, this covers the common code and not the platform-specific files.
Discussion is (as always) welcome. I have mixed feelings about some of this because it makes the code more difficult to read for humans (or those wired like me, I guess).
Especially the enum stuff seems stupid, the constants are specified as unsigned int, but enums are always signed int (before C23) so they still need to be cast back. Using const unsigned int or macros instead of enums would be an option, since these are private APIs.
@fabiensanglard you are particularly welcome to look at the libusb_get_ssplus_usb_device_capability_descriptor() changes.