Skip to contentSkip to conversation

bindings: rust: fix clippy lint warnings

PATCH In review Kent Gibson 3 participants 13 messages Jun 12, 2023 – Jun 19, 2023 GPIO

lore
Share & tools
lore
#0 lore raw

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
index eed2a42..0360333 100644
@@ -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
index 896596f..cac5659 100644
@@ -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(ret as 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
index f4de008..f160d23 100644
@@ -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
index d324ce6..64d20e2 100644
@@ -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
index 1deaf2b..89611bc 100644
@@ -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
index b0ceb3b..7651f1c 100644
@@ -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
index 26354e5..314e157 100644
@@ -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
index e973cde..405e675 100644
@@ -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_lines as 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 offset in &offsets[0..num_stored as usize] {
+        for offset in &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
index 702f1b4..bff9ece 100644
@@ -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
index b175eea..51cc7cd 100644
@@ -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_offsets as 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
index 918d6c2..3b1d2b6 100644
@@ -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
index 0c6c5c1..48edfaf 100644
@@ -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
index f264708..60b4ecc 100644
@@ -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
#1 lore raw
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

#2 lore raw
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

#3 lore raw
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?

#4 lore raw
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 like gpiod_line_config_get_num_configured_offsets not get translated to usize so 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 default

warning: 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.

#5 lore raw
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 like gpiod_line_config_get_num_configured_offsets not get translated to usize so 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/libgpiod directory, 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

#6 lore raw
In reply to Kent Gibson

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

#8 lore raw
In reply to Erik Schilling
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() in event_buffers.rs and 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.

#9 lore raw
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() in event_buffers.rs and 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

#10 lore raw
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.

#11 lore raw
In reply to Erik Schilling

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/
#12 lore raw
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/