bindings: rust: fix clippy lint warnings
PATCH 1/1 bindings: rust: fix clippy lint warnings
clippy from Rust 1.70 reports a host of warnings due to casting and type conversions across the FFI interface to libgpiod.
These casts and conversions are required to support old versions of Rust that do not support recent Rust FFI extensions.
Disable the warnings on a case by case basis, and move casting/conversion inside the corresponding unsafe section where appropriate to highlight that the FFI conversion is the cause.
clippy also finds a couple of genuine uneccessary casts, so remove those too.
---
There is also at least one instance of reformatting by rustfmt, but that seems appropriate too.
Cheers,
Kent.
bindings/rust/gpiosim-sys/src/lib.rs | 1 + bindings/rust/gpiosim-sys/src/sim.rs | 6 ++++-- bindings/rust/libgpiod/src/chip.rs | 5 ++++- bindings/rust/libgpiod/src/edge_event.rs | 1 + bindings/rust/libgpiod/src/event_buffer.rs | 3 +++ bindings/rust/libgpiod/src/info_event.rs | 1 + bindings/rust/libgpiod/src/lib.rs | 1 + bindings/rust/libgpiod/src/line_config.rs | 11 +++++++---- bindings/rust/libgpiod/src/line_info.rs | 1 + bindings/rust/libgpiod/src/line_request.rs | 14 +++++++++----- bindings/rust/libgpiod/src/line_settings.rs | 2 ++ bindings/rust/libgpiod/src/request_config.rs | 5 ++++- bindings/rust/libgpiod/tests/chip.rs | 2 +- 13 files changed, 39 insertions(+), 14 deletions(-)
bindings/rust/gpiosim-sys/src/lib.rs +1 -0
@@ -37,6 +37,7 @@ pub enum Value {impl Value {pub (crate )fn new (val:gpiosim_value ) ->Result <Self > { +#[allow(clippy::unnecessary_cast)] Ok (match val {GPIOSIM_VALUE_INACTIVE =>Value ::InActive ,GPIOSIM_VALUE_ACTIVE =>Value ::Active ,
bindings/rust/gpiosim-sys/src/sim.rs +4 -2
@@ -155,8 +155,9 @@ impl SimBank { } fn val(&self, offset: Offset) -> Result<Value> { + #[allow(clippy::unnecessary_cast)] // SAFETY: `gpiosim_bank` is guaranteed to be valid here. - let ret = unsafe { gpiosim_bank_get_value(self.bank, offset) }; + let ret = unsafe { gpiosim_bank_get_value(self.bank, offset) as i32}; if ret == -1 { Err(Error::OperationFailed(
@@ -164,7 +165,7 @@ impl SimBank { errno::errno(), )) }else { -Value ::new (retas i32 ) +Value ::new (ret) } }
@@ -185,6 +186,7 @@ impl SimBank { }fn set_num_lines (&self, num:usize ) ->Result <()> { +#[allow(clippy::useless_conversion)] // SAFETY: `gpiosim_bank` is guaranteed to be valid here. let ret =unsafe {gpiosim_bank_set_num_lines (self.bank, num.try_into ().unwrap ()) };if ret == -1 {
bindings/rust/libgpiod/src/chip.rs +4 -1
@@ -278,8 +278,11 @@ impl Info {/// Get the number of GPIO lines exposed by the chip. pub fn num_lines (&self) ->usize { +#[allow(clippy::unnecessary_cast)] // SAFETY: `gpiod_chip` is guaranteed to be valid here. -unsafe { gpiod::gpiod_chip_info_get_num_lines (self.info)as usize } +unsafe { + gpiod::gpiod_chip_info_get_num_lines (self.info)as usize + } } }
bindings/rust/libgpiod/src/edge_event.rs +1 -0
@@ -40,6 +40,7 @@ impl Event {/// Get the event type. pub fn event_type (&self) ->Result <EdgeKind > { +#[allow(clippy::unnecessary_cast)] // SAFETY: `gpiod_edge_event` is guaranteed to be valid here. EdgeKind ::new (unsafe { gpiod::gpiod_edge_event_get_event_type (self.0 ) }as u32 ) }
bindings/rust/libgpiod/src/event_buffer.rs +3 -0
@@ -54,6 +54,7 @@ impl<'a> Iterator for Events<'a> { }fn next (&mut self) ->Option <Self ::Item > { +#[allow(clippy::iter_nth_zero)] // as using next() is self referential self.nth (0 ) } }
@@ -81,6 +82,7 @@ impl Buffer { )); } +#[allow(clippy::unnecessary_cast)] // SAFETY: `gpiod_edge_event_buffer` is guaranteed to be valid here. let capacity =unsafe { gpiod::gpiod_edge_event_buffer_get_capacity (buffer)as usize };
@@ -103,6 +105,7 @@ impl Buffer { self.events[i] = ptr::null_mut (); } +#[allow(clippy::useless_conversion)] // SAFETY: `gpiod_line_request` is guaranteed to be valid here. let ret =unsafe { gpiod::gpiod_line_request_read_edge_events(
bindings/rust/libgpiod/src/info_event.rs +1 -0
@@ -33,6 +33,7 @@ impl Event {/// Get the event type of the status change event. pub fn event_type (&self) ->Result <InfoChangeKind > { +#[allow(clippy::unnecessary_cast)] // SAFETY: `gpiod_info_event` is guaranteed to be valid here. InfoChangeKind ::new (unsafe { gpiod::gpiod_info_event_get_event_type (self.event) }as u32 ) }
bindings/rust/libgpiod/src/lib.rs +1 -0
@@ -184,6 +184,7 @@ pub mod line {impl Value {pub fn new (val: gpiod::gpiod_line_value ) ->Result <Self > { +#[allow(clippy::unnecessary_cast)] Ok (match val {GPIOD_LINE_VALUE_INACTIVE =>Value ::InActive ,GPIOD_LINE_VALUE_ACTIVE =>Value ::Active ,
bindings/rust/libgpiod/src/line_config.rs +7 -4
@@ -106,20 +106,23 @@ impl Config {/// Get a mapping of offsets to line settings stored by this object. pub fn line_settings (&self) ->Result <SettingsMap > {let mut map =SettingsMap ::new (); +#[allow(clippy::unnecessary_cast)] // SAFETY: gpiod_line_config is guaranteed to be valid here -let num_lines =unsafe { gpiod::gpiod_line_config_get_num_configured_offsets (self.config) }; -let mut offsets =vec! [0 ; num_linesas usize ]; +let num_lines = +unsafe { gpiod::gpiod_line_config_get_num_configured_offsets (self.config)as usize }; +let mut offsets =vec! [0 ; num_lines]; +#[allow(clippy::unnecessary_cast)] // SAFETY: gpiod_line_config is guaranteed to be valid here. let num_stored =unsafe { gpiod::gpiod_line_config_get_configured_offsets ( self.config, offsets.as_mut_ptr (), num_lines, - ) + )as usize }; -for offsetin &offsets[0 ..num_storedas usize ] { +for offsetin &offsets[0 ..num_stored] {// SAFETY: `gpiod_line_config` is guaranteed to be valid here. let settings =unsafe { gpiod::gpiod_line_config_get_line_settings (self.config, *offset) };
bindings/rust/libgpiod/src/line_info.rs +1 -0
@@ -142,6 +142,7 @@ impl Info {/// Get the debounce period of the line. pub fn debounce_period (&self) ->Duration { +#[allow(clippy::unnecessary_cast)] // SAFETY: `gpiod_line_info` is guaranteed to be valid here. Duration ::from_micros (unsafe { gpiod::gpiod_line_info_get_debounce_period_us (self.info)as u64
bindings/rust/libgpiod/src/line_request.rs +9 -5
@@ -27,23 +27,27 @@ impl Request {/// Get the number of lines in the request. pub fn num_lines (&self) ->usize { +#[allow(clippy::unnecessary_cast)] // SAFETY: `gpiod_line_request` is guaranteed to be valid here. -unsafe { gpiod::gpiod_line_request_get_num_requested_lines (self.request)as usize } +unsafe { + gpiod::gpiod_line_request_get_num_requested_lines (self.request)as usize + } }/// Get the offsets of lines in the request. pub fn offsets (&self) ->Vec <Offset > { -let mut offsets =vec! [0 ; self.num_lines ()as usize ]; +let mut offsets =vec! [0 ; self.num_lines ()]; +#[allow(clippy::unnecessary_cast)] // SAFETY: `gpiod_line_request` is guaranteed to be valid here. let num_offsets =unsafe { gpiod::gpiod_line_request_get_requested_offsets ( self.request, offsets.as_mut_ptr (), self.num_lines (), - ) + )as usize }; - offsets.shrink_to (num_offsetsas usize ); + offsets.shrink_to (num_offsets); offsets }
@@ -145,7 +149,7 @@ impl Request {/// Set values of all lines associated with the request. pub fn set_values (&mut self, values: &[Value ]) ->Result <&mut Self > { -if values.len () != self.num_lines ()as usize { +if values.len () != self.num_lines () {return Err (Error ::InvalidArguments ); }
bindings/rust/libgpiod/src/line_settings.rs +2 -0
@@ -218,6 +218,7 @@ impl Settings {/// Get the debounce period. pub fn debounce_period (&self) ->Result <Duration > { +#[allow(clippy::unnecessary_cast)] // SAFETY: `gpiod_line_settings` is guaranteed to be valid here. Ok(Duration::from_micros(unsafe { gpiod::gpiod_line_settings_get_debounce_period_us(self.settings) as u64
@@ -243,6 +244,7 @@ impl Settings {/// Get the event clock setting. pub fn event_clock (&self) ->Result <EventClock > { +#[allow(clippy::unnecessary_cast)] // SAFETY: `gpiod_line_settings` is guaranteed to be valid here. EventClock ::new (unsafe { gpiod::gpiod_line_settings_get_event_clock (self.settings) }as u32 ) }
bindings/rust/libgpiod/src/request_config.rs +4 -1
@@ -82,8 +82,11 @@ impl Config {/// Get the edge event buffer size setting for the request config. pub fn event_buffer_size (&self) ->usize { +#[allow(clippy::unnecessary_cast)] // SAFETY: `gpiod_request_config` is guaranteed to be valid here. -unsafe { gpiod::gpiod_request_config_get_event_buffer_size (self.config)as usize } +unsafe { + gpiod::gpiod_request_config_get_event_buffer_size (self.config)as usize + } } }
bindings/rust/libgpiod/tests/chip.rs +1 -1
@@ -59,7 +59,7 @@ mod chip {assert_eq! (info.label ().unwrap (),LABEL );assert_eq! (info.name ().unwrap (), sim.chip_name ());assert_eq! (chip.path ().unwrap (), sim.dev_path ().to_str ().unwrap ()); -assert_eq! (info.num_lines (),NGPIO as usize ); +assert_eq! (info.num_lines (),NGPIO ); }#[test]
Signature · 1 line
2.40.1
Quoted 4 lines · Kent Gibson
clippy from Rust 1.70 reports a host of warnings due to casting and type conversions across the FFI interface to libgpiod.
These casts and conversions are required to support old versions of Rust that do not support recent Rust FFI extensions.
Could you elaborate which extensions are relevant here? Would it be realistic to just update the minimum Rust version instead of needing to include these suppression directives?
- Erik
Quoted 10 lines · Erik Schilling
Quoted 4 lines · Kent Gibson
clippy from Rust 1.70 reports a host of warnings due to casting and type conversions across the FFI interface to libgpiod.
These casts and conversions are required to support old versions of Rust that do not support recent Rust FFI extensions.Could you elaborate which extensions are relevant here? Would it be realistic to just update the minimum Rust version instead of needing to include these suppression directives?
Types were added in core::ffi[1] in 1.64 for just this purpose. e.g. c_uint[2]
Though c_size_t[3] still remains in Experimental.
And I guess the clippy lints followed soon after.
Wrt setting the MSRV, but I assumed not, hence the allows.
Cheers,
Kent.
[1] https://doc.rust-lang.org/stable/core/ffi/index.html
[2] https://doc.rust-lang.org/stable/core/ffi/type.c_uint.html
[3] https://doc.rust-lang.org/stable/core/ffi/type.c_size_t.html
Quoted 12 lines · Kent Gibson
Quoted 10 lines · Erik Schilling
Quoted 4 lines · Kent Gibson
clippy from Rust 1.70 reports a host of warnings due to casting and type conversions across the FFI interface to libgpiod.
These casts and conversions are required to support old versions of Rust that do not support recent Rust FFI extensions.Could you elaborate which extensions are relevant here? Would it be realistic to just update the minimum Rust version instead of needing to include these suppression directives?
Quoted 7 lines
Types were added in core::ffi[1] in 1.64 for just this purpose. e.g. c_uint[2]
Though c_size_t[3] still remains in Experimental.And I guess the clippy lints followed soon after.
Wrt setting the MSRV, but I assumed not, hence the allows.
For me bindgen seems to generate usize of size_t, thats why I asked. Does that depend on the Rust version somehow? Or more concretely:
When will things like gpiod_line_config_get_num_configured_offsets not get translated to usize so that we need a cast?
On my end (with latest toolchain and nightly), I do not see any clippy warnings with cargo clippy. How exactly did you produce those warnings?
Quoted 17 lines · Erik Schilling
Quoted 16 lines · Kent Gibson
Quoted 10 lines · Erik Schilling
Quoted 4 lines · Kent Gibson
clippy from Rust 1.70 reports a host of warnings due to casting and type conversions across the FFI interface to libgpiod.
These casts and conversions are required to support old versions of Rust that do not support recent Rust FFI extensions.Could you elaborate which extensions are relevant here? Would it be realistic to just update the minimum Rust version instead of needing to include these suppression directives?
Types were added in core::ffi[1] in 1.64 for just this purpose. e.g. c_uint[2]
Though c_size_t[3] still remains in Experimental.
Quoted 9 lines
Quoted 3 lines
And I guess the clippy lints followed soon after.
Wrt setting the MSRV, but I assumed not, hence the allows.
For me bindgen seems to generate usize of
size_t, thats why I asked. Does that depend on the Rust version somehow? Or more concretely:
When will things likegpiod_line_config_get_num_configured_offsetsnot get translated tousizeso that we need a cast?
No idea - outside my area.
Quoted 4 lines
On my end (with latest toolchain and nightly), I do not see any clippy warnings with
cargo clippy. How exactly did you produce those warnings?
Interesting. With stable on libgpiod master in the rust/libgpiod directory, and with these in my environment:
export SYSTEM_DEPS_LIBGPIOD_NO_PKG_CONFIG=1
export SYSTEM_DEPS_LIBGPIOD_SEARCH_NATIVE="${PWD}/../../../lib/.libs/" export SYSTEM_DEPS_LIBGPIOD_LIB=gpiod
export SYSTEM_DEPS_LIBGPIOD_INCLUDE="${PWD}/../../../include/"
I get:
$ cargo clippy --tests
warning: casting to the same type is unnecessary (`u32` -> `u32`)
--> libgpiod/src/info_event.rs:37:29
|
37 | InfoChangeKind::new(unsafe { gpiod::gpiod_info_event_get_event_type(self.event) } as u32)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unsafe { gpiod::gpiod_info_event_get_event_type(self.event) }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
= note: `#[warn(clippy::unnecessary_cast)]` on by defaultwarning: casting to the same type is unnecessary (usize -> usize)
--> libgpiod/src/chip.rs:282:18
|
282 | unsafe { gpiod::gpiod_chip_info_get_num_lines(self.info) as usize }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `gpiod::gpiod_chip_info_get_num_lines(self.info)`
|= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
warning: casting to the same type is unnecessary (u32 -> u32)
--> libgpiod/src/edge_event.rs:44:23
|
44 | EdgeKind::new(unsafe { gpiod::gpiod_edge_event_get_event_type(self.0) } as u32)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unsafe { gpiod::gpiod_edge_event_get_event_type(self.0) }`
|= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
warning: called .nth(0) on a std::iter::Iterator, when .next() is equivalent
--> libgpiod/src/event_buffer.rs:57:9
| 57 | self.nth(0) | ^^^^^^^^^^^ help: try calling `.next()` instead of `.nth(0)`: `self.next()` |
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#iter_nth_zero
= note: #[warn(clippy::iter_nth_zero)] on by default
warning: casting to the same type is unnecessary (usize -> usize)
--> libgpiod/src/event_buffer.rs:85:33
|
85 | let capacity = unsafe { gpiod::gpiod_edge_event_buffer_get_capacity(buffer) as usize };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `gpiod::gpiod_edge_event_buffer_get_capacity(buffer)`
|= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
warning: useless conversion to the same type: usize
--> libgpiod/src/event_buffer.rs:111:17
|
111 | self.events.len().try_into().unwrap(),
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| = help: consider removing .try_into()
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
= note: #[warn(clippy::useless_conversion)] on by default
warning: casting to the same type is unnecessary (usize -> usize)
--> libgpiod/src/line_request.rs:31:18
|
31 | unsafe { gpiod::gpiod_line_request_get_num_requested_lines(self.request) as usize }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `gpiod::gpiod_line_request_get_num_requested_lines(self.request)`
|= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
warning: casting to the same type is unnecessary (usize -> usize)
--> libgpiod/src/line_request.rs:36:35
| 36 | let mut offsets = vec![0; self.num_lines() as usize]; | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `self.num_lines()` |
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
warning: casting to the same type is unnecessary (usize -> usize)
--> libgpiod/src/line_request.rs:46:27
| 46 | offsets.shrink_to(num_offsets as usize); | ^^^^^^^^^^^^^^^^^^^^ help: try: `num_offsets` |
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
warning: casting to the same type is unnecessary (usize -> usize)
--> libgpiod/src/line_request.rs:148:28
|
148 | if values.len() != self.num_lines() as usize {
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `self.num_lines()`
|= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
warning: casting to the same type is unnecessary (usize -> usize)
--> libgpiod/src/request_config.rs:86:18
|
86 | unsafe { gpiod::gpiod_request_config_get_event_buffer_size(self.config) as usize }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `gpiod::gpiod_request_config_get_event_buffer_size(self.config)`
|= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
warning: casting to the same type is unnecessary (usize -> usize)
--> libgpiod/src/line_config.rs:111:35
|
111 | let mut offsets = vec![0; num_lines as usize];
| ^^^^^^^^^^^^^^^^^^ help: try: `num_lines`
|= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
warning: casting to the same type is unnecessary (usize -> usize)
--> libgpiod/src/line_config.rs:122:35
|
122 | for offset in &offsets[0..num_stored as usize] {
| ^^^^^^^^^^^^^^^^^^^ help: try: `num_stored`
|= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
warning: casting to the same type is unnecessary (u64 -> u64)
--> libgpiod/src/line_info.rs:147:13
|
147 | gpiod::gpiod_line_info_get_debounce_period_us(self.info) as u64
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `gpiod::gpiod_line_info_get_debounce_period_us(self.info)`
|= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
warning: casting to the same type is unnecessary (u64 -> u64)
--> libgpiod/src/line_settings.rs:223:13
|
223 | gpiod::gpiod_line_settings_get_debounce_period_us(self.settings) as u64
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `gpiod::gpiod_line_settings_get_debounce_period_us(self.settings)`
|= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
warning: casting to the same type is unnecessary (u32 -> u32)
--> libgpiod/src/line_settings.rs:247:25
|
247 | EventClock::new(unsafe { gpiod::gpiod_line_settings_get_event_clock(self.settings) } as u32)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unsafe { gpiod::gpiod_line_settings_get_event_clock(self.settings) }`
|= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
warning: casting to the same type is unnecessary (i32 -> i32)
--> libgpiod/src/lib.rs:196:66
|
196 | _ => return Err(Error::InvalidEnumValue("Value", val as i32)),
| ^^^^^^^^^^ help: try: `val`
|= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
warning: libgpiod (lib) generated 17 warnings (run cargo clippy --fix --lib -p libgpiod to apply 16 suggestions)
warning: casting to the same type is unnecessary (i32 -> i32)
--> gpiosim-sys/src/sim.rs:167:24
|
167 | Value::new(ret as i32)
| ^^^^^^^^^^ help: try: `ret`
| = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
= note: #[warn(clippy::unnecessary_cast)] on by default
warning: useless conversion to the same type: usize
--> gpiosim-sys/src/sim.rs:189:66
|
189 | let ret = unsafe { gpiosim_bank_set_num_lines(self.bank, num.try_into().unwrap()) };
| ^^^^^^^^^^^^^^
| = help: consider removing .try_into()
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
= note: #[warn(clippy::useless_conversion)] on by default
warning: casting to the same type is unnecessary (i32 -> i32)
--> gpiosim-sys/src/lib.rs:49:62
|
49 | _ => return Err(Error::InvalidEnumValue("Value", val as i32)),
| ^^^^^^^^^^ help: try: `val`
|= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
warning: gpiosim-sys (lib) generated 3 warnings (run cargo clippy --fix --lib -p gpiosim-sys to apply 2 suggestions)
warning: casting to the same type is unnecessary (usize -> usize)
--> libgpiod/tests/chip.rs:62:42
|
62 | assert_eq!(info.num_lines(), NGPIO as usize);
| ^^^^^^^^^^^^^^ help: try: `{ NGPIO }`
| = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
= note: #[warn(clippy::unnecessary_cast)] on by default
warning: libgpiod (lib test) generated 17 warnings (17 duplicates)
warning: libgpiod (test "chip") generated 1 warning (run cargo clippy --fix --test "chip" to apply 1 suggestion)
Finished dev [unoptimized + debuginfo] target(s) in 0.03s
where stable is:
stable-x86_64-unknown-linux-gnu unchanged - rustc 1.70.0 (90c541806 2023-05-31)
I get the same from nightly.
nightly-x86_64-unknown-linux-gnu unchanged - rustc 1.72.0-nightly (dd5d7c729 2023-06-02)
The --tests is to extend the check to the tests - you get the vast majority of those without it.
In both cases the patched version is clean.
Cheers,
Kent.
Quoted 30 lines · Kent Gibson
Quoted 17 lines · Erik Schilling
Quoted 16 lines · Kent Gibson
Quoted 10 lines · Erik Schilling
Quoted 4 lines · Kent Gibson
clippy from Rust 1.70 reports a host of warnings due to casting and type conversions across the FFI interface to libgpiod.
These casts and conversions are required to support old versions of Rust that do not support recent Rust FFI extensions.Could you elaborate which extensions are relevant here? Would it be realistic to just update the minimum Rust version instead of needing to include these suppression directives?
Types were added in core::ffi[1] in 1.64 for just this purpose. e.g. c_uint[2]
Though c_size_t[3] still remains in Experimental.Quoted 9 lines
Quoted 3 lines
And I guess the clippy lints followed soon after.
Wrt setting the MSRV, but I assumed not, hence the allows.
For me bindgen seems to generate usize of
size_t, thats why I asked. Does that depend on the Rust version somehow? Or more concretely:
When will things likegpiod_line_config_get_num_configured_offsetsnot get translated tousizeso that we need a cast?No idea - outside my area.
Quoted 7 lines
Quoted 4 lines
On my end (with latest toolchain and nightly), I do not see any clippy warnings with
cargo clippy. How exactly did you produce those warnings?Interesting. With stable on libgpiod master in the
rust/libgpioddirectory, and with these in my environment:
Oh... My fault... I saw the -sys somewhere and tried to run things in libgpiod-sys (after inspecting the generated bindings). Need to revisit this in a calm moment.
Thanks for the additional detail!
- Erik
Sorry, got distracted while sorting out the MSRV mess that I sent a separate patch for [4].
Quoted 12 lines · Kent Gibson
Quoted 10 lines · Erik Schilling
Quoted 4 lines · Kent Gibson
clippy from Rust 1.70 reports a host of warnings due to casting and type conversions across the FFI interface to libgpiod.
These casts and conversions are required to support old versions of Rust that do not support recent Rust FFI extensions.Could you elaborate which extensions are relevant here? Would it be realistic to just update the minimum Rust version instead of needing to include these suppression directives?
Quoted 7 lines
Types were added in core::ffi[1] in 1.64 for just this purpose. e.g. c_uint[2]
Though c_size_t[3] still remains in Experimental.[1] https://doc.rust-lang.org/stable/core/ffi/index.html
[2] https://doc.rust-lang.org/stable/core/ffi/type.c_uint.html
[3] https://doc.rust-lang.org/stable/core/ffi/type.c_size_t.html
I do not think that this is the reason why we need the casts... bindgen generates bindings using std::os::raw::c_uint [5] which is stable since 1.1.0 (and was previously defined as u32 [6]). I think we can just drop the casts entirely? I can run cargo clippy --fix on latest stable (1.70.0), then go back to 1.60 and everything is still building. I am having trouble to execute the tests in that version due to some linkage errors, but that should not be the fault of the casts.
Did I got this correct or am I misunderstanding your reasoning?
Note: One needs to fix a bug that cargo clippy --fix introduces since it replaces nth(0) with next() in event_buffers.rs and introduces a unconditional recursion.
[4] https://lore.kernel.org/r/[email protected]/
[5] https://doc.rust-lang.org/stable/std/os/raw/type.c_uint.html
[6] https://doc.rust-lang.org/1.60.0/std/os/raw/type.c_uint.html
- Erik
Quoted 3 lines
Note: One needs to fix a bug that cargo clippy --fix introduces since it replaces nth(0) with
next()inevent_buffers.rsand introduces a unconditional recursion.
This seems to be a known false-positive:
https://github.com/rust-lang/rust-clippy/issues/9820
- Erik
Quoted 11 lines · Erik Schilling
Sorry, got distracted while sorting out the MSRV mess that I sent a separate patch for [4].
I do not think that this is the reason why we need the casts... bindgen generates bindings using
std::os::raw::c_uint[5] which is stable since 1.1.0 (and was previously defined as u32 [6]). I think we can just drop the casts entirely? I can run cargo clippy --fix on latest stable (1.70.0), then go back to 1.60 and everything is still building. I am having trouble to execute the tests in that version due to some linkage errors, but that should not be the fault of the casts.
Quoted 2 lines
Did I got this correct or am I misunderstanding your reasoning?
My reasoning was simply that building the bindings as you suggested resulted in lint warnings, which is noisy and iritating when trying to lint my own code. And I assumed that changing the code or limiting the rust version was not an option.
But I'm just the messenger. Your question would be better directed at Viresh - it is his code so he should be able to tell you why the casts are there.
IIRC we needed the casts historically, though I don't recall the rust version we were using at the time.
If we've moved beyond that then I have no problem with the casts being removedi, in fact in my initial comment I lamented the fact they were necessary.
Quoted 4 lines
Note: One needs to fix a bug that cargo clippy --fix introduces since it replaces nth(0) with
next()inevent_buffers.rsand introduces a unconditional recursion.
Who is using --fix??
I did put an allow in there for that one, with a comment about the recursion, though I'm not sure the comment is sufficiently clear without the warning in front of you - and you no longer get that with the allow in place.
Cheers,
Kent.
Quoted 12 lines · Kent Gibson
Quoted 11 lines · Erik Schilling
Sorry, got distracted while sorting out the MSRV mess that I sent a separate patch for [4].
I do not think that this is the reason why we need the casts... bindgen generates bindings using
std::os::raw::c_uint[5] which is stable since 1.1.0 (and was previously defined as u32 [6]). I think we can just drop the casts entirely? I can run cargo clippy --fix on latest stable (1.70.0), then go back to 1.60 and everything is still building. I am having trouble to execute the tests in that version due to some linkage errors, but that should not be the fault of the casts.
Quoted 6 lines
Quoted 2 lines
Did I got this correct or am I misunderstanding your reasoning?
My reasoning was simply that building the bindings as you suggested resulted in lint warnings, which is noisy and iritating when trying to lint my own code.
I fully agree that we should fix them! I was just confused about the explanation.
Quoted 8 lines
But I'm just the messenger. Your question would be better directed at Viresh - it is his code so he should be able to tell you why the casts are there.
IIRC we needed the casts historically, though I don't recall the rust version we were using at the time.
If we've moved beyond that then I have no problem with the casts being removedi, in fact in my initial comment I lamented the fact they were necessary.
ACK. Not sure how the situation was when the patches came together, but today I would suggest to just drop the casts. Do you want to respin your series or shall I send the fixes?
Quoted 6 lines
Quoted 4 lines
Note: One needs to fix a bug that cargo clippy --fix introduces since it replaces nth(0) with
next()inevent_buffers.rsand introduces a unconditional recursion.Who is using --fix??
I do all the time (and spend the time I saved typing on reviewing)! (Thought mostly I use the suggested actions suggested by the LSP in the editor instead of the CLI tools)
Quoted 4 lines
I did put an allow in there for that one, with a comment about the recursion, though I'm not sure the comment is sufficiently clear without the warning in front of you - and you no longer get that with the allow in place.
Ah. Did not notice that... My brain was only thinking about the casts.
- Erik
Quoted 22 lines · Erik Schilling
Quoted 8 lines · Kent Gibson
Quoted 3 lines · Erik Schilling
Sorry, got distracted while sorting out the MSRV mess that I sent a separate patch for [4].
My reasoning was simply that building the bindings as you suggested resulted in lint warnings, which is noisy and iritating when trying to lint my own code.
I fully agree that we should fix them! I was just confused about the explanation.
Quoted 8 lines
But I'm just the messenger. Your question would be better directed at Viresh - it is his code so he should be able to tell you why the casts are there.
IIRC we needed the casts historically, though I don't recall the rust version we were using at the time.
If we've moved beyond that then I have no problem with the casts being removed, in fact in my initial comment I lamented the fact they were necessary.
Quoted 4 lines
ACK. Not sure how the situation was when the patches came together, but today I would suggest to just drop the casts. Do you want to respin your series or shall I send the fixes?
You go ahead.
Cheers,
Kent.
I missed these earlier, thanks for cc'ing me now Kent.
Quoted 16 lines · Erik Schilling
Quoted 3 lines · Kent Gibson
My reasoning was simply that building the bindings as you suggested resulted in lint warnings, which is noisy and iritating when trying to lint my own code.
I fully agree that we should fix them! I was just confused about the explanation.
Quoted 8 lines
But I'm just the messenger. Your question would be better directed at Viresh - it is his code so he should be able to tell you why the casts are there.
IIRC we needed the casts historically, though I don't recall the rust version we were using at the time.
If we've moved beyond that then I have no problem with the casts being removedi, in fact in my initial comment I lamented the fact they were necessary.
I don't remember the versions used at that time, but here [1] are few of the auto-generated FFI bindings that I used to keep in my series then, in case these can help.
IIRC, some of these were required for 32 bit builds. Don't remember the exact details but there were build / clippy errors / warnings without them.
I am fine with updating code based on latest version of Rust and get these removed.
Erik: Please build for 32 bit ARM too btw.
Signature · 3 lines
viresh [1] https://lore.kernel.org/all/401d9417d895b8b1b19ca577c84347d89f7e0fbd.1667815011.git.viresh.kumar@linaro.org/
Quoted 24 lines · Viresh Kumar
I missed these earlier, thanks for cc'ing me now Kent.
Quoted 16 lines · Erik Schilling
Quoted 3 lines · Kent Gibson
My reasoning was simply that building the bindings as you suggested resulted in lint warnings, which is noisy and iritating when trying to lint my own code.
I fully agree that we should fix them! I was just confused about the explanation.
Quoted 8 lines
But I'm just the messenger. Your question would be better directed at Viresh - it is his code so he should be able to tell you why the casts are there.
IIRC we needed the casts historically, though I don't recall the rust version we were using at the time.
If we've moved beyond that then I have no problem with the casts being removedi, in fact in my initial comment I lamented the fact they were necessary.I don't remember the versions used at that time, but here [1] are few of the auto-generated FFI bindings that I used to keep in my series then, in case these can help.
Quoted 8 lines
IIRC, some of these were required for 32 bit builds. Don't remember the exact details but there were build / clippy errors / warnings without them.
I am fine with updating code based on latest version of Rust and get these removed.
Erik: Please build for 32 bit ARM too btw.
Will do. Will try to come up with some test suite that may be upstreamable too.
Quoted 5 lines
Signature · 4 lines
-- viresh [1] https://lore.kernel.org/all/401d9417d895b8b1b19ca577c84347d89f7e0fbd.1667815011.git.viresh.kumar@linaro.org/