From 6b524dc74dbbd2c8b519300f041c297ddde86695 Mon Sep 17 00:00:00 2001 From: Alex Crawford Date: Fri, 25 Dec 2020 23:56:39 -0800 Subject: [PATCH 1/5] Various cleanups These were flagged by `cargo clippy`: warning: using `clone` on a `Copy` type warning: passing a unit value to a function warning: redundant closure found warning: called `iter().cloned().collect()` on a slice to create a `Vec`. Calling `to_vec()` is both faster and more readable --- src/iface/ethernet.rs | 4 ++-- src/lib.rs | 1 + src/socket/raw.rs | 10 +++++----- src/storage/ring_buffer.rs | 2 +- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/iface/ethernet.rs b/src/iface/ethernet.rs index 7a5d73d..7e12548 100644 --- a/src/iface/ethernet.rs +++ b/src/iface/ethernet.rs @@ -1757,7 +1757,7 @@ mod test { let mut pkts = Vec::new(); while let Some((rx, _tx)) = iface.device.receive() { rx.consume(timestamp, |pkt| { - pkts.push(pkt.iter().cloned().collect()); + pkts.push(pkt.to_vec()); Ok(()) }).unwrap(); } @@ -2567,7 +2567,7 @@ mod test { // Leave multicast groups let timestamp = Instant::now(); for group in &groups { - iface.leave_multicast_group(group.clone(), timestamp) + iface.leave_multicast_group(*group, timestamp) .unwrap(); } diff --git a/src/lib.rs b/src/lib.rs index 22bd6c4..0eb50e4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -94,6 +94,7 @@ compile_error!("at least one socket needs to be enabled"); */ #![allow(clippy::redundant_field_names)] #![allow(clippy::identity_op)] #![allow(clippy::option_map_unit_fn)] +#![allow(clippy::unit_arg)] #[cfg(feature = "alloc")] extern crate alloc; diff --git a/src/socket/raw.rs b/src/socket/raw.rs index 56b66d5..61b771d 100644 --- a/src/socket/raw.rs +++ b/src/socket/raw.rs @@ -473,14 +473,14 @@ mod test { { let mut socket = ipv4_locals::socket(buffer(0), buffer(2)); - let mut wrong_version = ipv4_locals::PACKET_BYTES.clone(); + let mut wrong_version = ipv4_locals::PACKET_BYTES; Ipv4Packet::new_unchecked(&mut wrong_version).set_version(6); assert_eq!(socket.send_slice(&wrong_version[..]), Ok(())); assert_eq!(socket.dispatch(&checksum_caps, |_| unreachable!()), Ok(())); - let mut wrong_protocol = ipv4_locals::PACKET_BYTES.clone(); + let mut wrong_protocol = ipv4_locals::PACKET_BYTES; Ipv4Packet::new_unchecked(&mut wrong_protocol).set_protocol(IpProtocol::Tcp); assert_eq!(socket.send_slice(&wrong_protocol[..]), Ok(())); @@ -491,14 +491,14 @@ mod test { { let mut socket = ipv6_locals::socket(buffer(0), buffer(2)); - let mut wrong_version = ipv6_locals::PACKET_BYTES.clone(); + let mut wrong_version = ipv6_locals::PACKET_BYTES; Ipv6Packet::new_unchecked(&mut wrong_version[..]).set_version(4); assert_eq!(socket.send_slice(&wrong_version[..]), Ok(())); assert_eq!(socket.dispatch(&checksum_caps, |_| unreachable!()), Ok(())); - let mut wrong_protocol = ipv6_locals::PACKET_BYTES.clone(); + let mut wrong_protocol = ipv6_locals::PACKET_BYTES; Ipv6Packet::new_unchecked(&mut wrong_protocol[..]).set_next_header(IpProtocol::Tcp); assert_eq!(socket.send_slice(&wrong_protocol[..]), Ok(())); @@ -514,7 +514,7 @@ mod test { let mut socket = ipv4_locals::socket(buffer(1), buffer(0)); assert!(!socket.can_recv()); - let mut cksumd_packet = ipv4_locals::PACKET_BYTES.clone(); + let mut cksumd_packet = ipv4_locals::PACKET_BYTES; Ipv4Packet::new_unchecked(&mut cksumd_packet).fill_checksum(); assert_eq!(socket.recv(), Err(Error::Exhausted)); diff --git a/src/storage/ring_buffer.rs b/src/storage/ring_buffer.rs index 6ea46ac..063c9f3 100644 --- a/src/storage/ring_buffer.rs +++ b/src/storage/ring_buffer.rs @@ -405,7 +405,7 @@ mod test { assert_eq!(ring.dequeue_one_with(|_| unreachable!()) as Result<()>, Err(Error::Exhausted)); - ring.enqueue_one_with(|e| Ok(e)).unwrap(); + ring.enqueue_one_with(Ok).unwrap(); assert!(!ring.is_empty()); assert!(!ring.is_full()); From ea4579d68aa36b72200936a19a953c9c89e0e6a1 Mon Sep 17 00:00:00 2001 From: Alex Crawford Date: Tue, 29 Dec 2020 18:40:28 -0800 Subject: [PATCH 2/5] Clean up examples These were flagged by `cargo clippy`: warning: you seem to be trying to use match for destructuring a single pattern. Consider using `if let` warning: called `.nth(0)` on a `std::iter::Iterator`, when `.next()` is equivalent warning: using `write!()` with a format string that ends in a single newline warning: useless conversion to the same type: `smoltcp::wire::Ipv4Address` warning: called `map(f)` on an `Option` value where `f` is a closure that returns the unit type `()` warning: returning the result of a `let` binding from a block warning: use of `unwrap_or` followed by a function call --- examples/dhcp_client.rs | 10 +++++----- examples/loopback.rs | 4 +--- examples/multicast.rs | 2 +- examples/ping.rs | 2 +- examples/server.rs | 2 +- examples/utils.rs | 4 ++-- 6 files changed, 11 insertions(+), 13 deletions(-) diff --git a/examples/dhcp_client.rs b/examples/dhcp_client.rs index daabc9c..7fa009c 100644 --- a/examples/dhcp_client.rs +++ b/examples/dhcp_client.rs @@ -1,3 +1,4 @@ +#![allow(clippy::option_map_unit_fn)] mod utils; use std::collections::BTreeMap; @@ -57,10 +58,10 @@ fn main() { }); config.map(|config| { println!("DHCP config: {:?}", config); - match config.address { - Some(cidr) => if cidr != prev_cidr { + if let Some(cidr) = config.address { + if cidr != prev_cidr { iface.update_ip_addrs(|addrs| { - addrs.iter_mut().nth(0) + addrs.iter_mut().next() .map(|addr| { *addr = IpCidr::Ipv4(cidr); }); @@ -68,11 +69,10 @@ fn main() { prev_cidr = cidr; println!("Assigned a new IPv4 address: {}", cidr); } - _ => {} } config.router.map(|router| iface.routes_mut() - .add_default_ipv4_route(router.into()) + .add_default_ipv4_route(router) .unwrap() ); iface.routes_mut() diff --git a/examples/loopback.rs b/examples/loopback.rs index 9d0c39b..c84f286 100644 --- a/examples/loopback.rs +++ b/examples/loopback.rs @@ -76,9 +76,7 @@ fn main() { utils::add_middleware_options(&mut opts, &mut free); let mut matches = utils::parse_options(&opts, free); - let device = utils::parse_middleware_options(&mut matches, device, /*loopback=*/true); - - device + utils::parse_middleware_options(&mut matches, device, /*loopback=*/true) }; let mut neighbor_cache_entries = [None; 8]; diff --git a/examples/multicast.rs b/examples/multicast.rs index 92f1876..e637467 100644 --- a/examples/multicast.rs +++ b/examples/multicast.rs @@ -83,7 +83,7 @@ fn main() { // For display purposes only - normally we wouldn't process incoming IGMP packets // in the application layer socket.recv() - .and_then(|payload| Ipv4Packet::new_checked(payload)) + .and_then(Ipv4Packet::new_checked) .and_then(|ipv4_packet| IgmpPacket::new_checked(ipv4_packet.payload())) .and_then(|igmp_packet| IgmpRepr::parse(&igmp_packet)) .map(|igmp_repr| println!("IGMP packet: {:?}", igmp_repr)) diff --git a/examples/ping.rs b/examples/ping.rs index 411e450..1d290d4 100644 --- a/examples/ping.rs +++ b/examples/ping.rs @@ -74,7 +74,7 @@ fn main() { let count = matches.opt_str("count").map(|s| usize::from_str(&s).unwrap()).unwrap_or(4); let interval = matches.opt_str("interval") .map(|s| Duration::from_secs(u64::from_str(&s).unwrap())) - .unwrap_or(Duration::from_secs(1)); + .unwrap_or_else(|| Duration::from_secs(1)); let timeout = Duration::from_secs( matches.opt_str("timeout").map(|s| u64::from_str(&s).unwrap()).unwrap_or(5) ); diff --git a/examples/server.rs b/examples/server.rs index 572c84a..95675c7 100644 --- a/examples/server.rs +++ b/examples/server.rs @@ -109,7 +109,7 @@ fn main() { if socket.can_send() { debug!("tcp:6969 send greeting"); - write!(socket, "hello\n").unwrap(); + writeln!(socket, "hello").unwrap(); debug!("tcp:6969 close"); socket.close(); } diff --git a/examples/utils.rs b/examples/utils.rs index 09d43d4..02b2de0 100644 --- a/examples/utils.rs +++ b/examples/utils.rs @@ -42,7 +42,7 @@ pub fn setup_logging_with_clock(filter: &str, since_startup: F) }) .filter(None, LevelFilter::Trace) .parse(filter) - .parse(&env::var("RUST_LOG").unwrap_or("".to_owned())) + .parse(&env::var("RUST_LOG").unwrap_or_else(|_| "".to_owned())) .init(); } @@ -68,7 +68,7 @@ pub fn parse_options(options: &Options, free: Vec<&str>) -> Matches { Ok(matches) => { if matches.opt_present("h") || matches.free.len() != free.len() { let brief = format!("Usage: {} [OPTION]... {}", - env::args().nth(0).unwrap(), free.join(" ")); + env::args().next().unwrap(), free.join(" ")); print!("{}", options.usage(&brief)); process::exit(if matches.free.len() != free.len() { 1 } else { 0 }) } From cb66f9f03677e7119d9243b60717cce128ed0b5d Mon Sep 17 00:00:00 2001 From: Alex Crawford Date: Mon, 4 Jan 2021 10:06:57 -0800 Subject: [PATCH 3/5] Allow DeviceCapabilities to be initialized This was flagged by `cargo clippy`: warning: field assignment outside of initializer for an instance created with Default::default() This changes the visibility of the dummy field to be public, but only to the crate. --- src/phy/mod.rs | 2 +- src/socket/tcp.rs | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/phy/mod.rs b/src/phy/mod.rs index 8ef6991..adf0597 100644 --- a/src/phy/mod.rs +++ b/src/phy/mod.rs @@ -217,7 +217,7 @@ pub struct DeviceCapabilities { /// Only present to prevent people from trying to initialize every field of DeviceLimits, /// which would not let us add new fields in the future. - dummy: () + pub(crate) dummy: () } /// An interface for sending and receiving raw network frames. diff --git a/src/socket/tcp.rs b/src/socket/tcp.rs index 63a6903..0d2d571 100644 --- a/src/socket/tcp.rs +++ b/src/socket/tcp.rs @@ -1972,8 +1972,10 @@ mod test { fn recv(socket: &mut TcpSocket, timestamp: Instant, mut f: F) where F: FnMut(Result) { - let mut caps = DeviceCapabilities::default(); - caps.max_transmission_unit = 1520; + let caps = DeviceCapabilities { + max_transmission_unit: 1520, + ..Default::default() + }; let result = socket.dispatch(timestamp, &caps, |(ip_repr, tcp_repr)| { let ip_repr = ip_repr.lower(&[IpCidr::new(LOCAL_END.addr, 24)]).unwrap(); @@ -4821,8 +4823,10 @@ mod test { #[test] fn test_set_hop_limit() { let mut s = socket_syn_received(); - let mut caps = DeviceCapabilities::default(); - caps.max_transmission_unit = 1520; + let caps = DeviceCapabilities { + max_transmission_unit: 1520, + ..Default::default() + }; s.set_hop_limit(Some(0x2a)); assert_eq!(s.dispatch(Instant::from_millis(0), &caps, |(ip_repr, _)| { From bcd78bbb1b55678af763510b6699206e83638653 Mon Sep 17 00:00:00 2001 From: Alex Crawford Date: Tue, 29 Dec 2020 18:55:35 -0800 Subject: [PATCH 4/5] Enable clippy on tests and examples Might as well run the lints on our tests and examples. When I first started doing this cleanup, I thought this was the default, but I must have run `cargo clippy --all-targets` at some point in there. --- .github/workflows/clippy.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/clippy.yml b/.github/workflows/clippy.yml index 81848f9..76985f4 100644 --- a/.github/workflows/clippy.yml +++ b/.github/workflows/clippy.yml @@ -23,4 +23,4 @@ jobs: - uses: actions-rs/clippy-check@v1 with: token: ${{ secrets.GITHUB_TOKEN }} - args: -- -D warnings + args: --tests --examples -- -D warnings From 1ca765230dec8d5c8d6cce57f190faf0ef211659 Mon Sep 17 00:00:00 2001 From: Alex Crawford Date: Mon, 4 Jan 2021 10:42:26 -0800 Subject: [PATCH 5/5] Pin clippy check to 1.49.0 It can be rather surprising when new lints pop up when a new stable toolchain is released. Let's pin this check to a specific version to avoid those surprises. --- .github/workflows/clippy.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/clippy.yml b/.github/workflows/clippy.yml index 76985f4..585aab7 100644 --- a/.github/workflows/clippy.yml +++ b/.github/workflows/clippy.yml @@ -17,7 +17,7 @@ jobs: - uses: actions-rs/toolchain@v1 with: profile: minimal - toolchain: stable + toolchain: 1.49.0 override: true components: clippy - uses: actions-rs/clippy-check@v1