Address clippy lints #160

lopopolo · 2023-05-21T17:24:22Z

This PR addresses clippy lint violations, one violation at a time, by working through cargo clippy output. This uses the default warn set of warnings in clippy.

I did run cargo clippy --fix -- --warn clippy::pedantic to see what it would do and it converted a lot of pointer as casts to ptr.cast::<_>() calls. I find this increases readability and is less footgun-y in that it preserves the constness of pointers, but I did not include these changes in this PR.

I did not include any changes to the CI pipeline in this PR.

See #158.

lopopolo · 2023-05-21T21:45:19Z

src/unicode/word.rs

@@ -142,7 +137,7 @@ impl<'a> Iterator for WordIndices<'a> {

    #[inline]
    fn next(&mut self) -> Option<(usize, usize, &'a str)> {
-        while let Some((start, end, word)) = self.0.next() {
+        for (start, end, word) in self.0.by_ref() {


This appears it can have the same Iterator::find simplification performed on it as the above hunk of the diff.

This is a pedantic lint, but it removes a transmute where a cast will do. The code is safe for the same reasons the transmute is safe: `BStr` is a repr transparent wrapper around `[u8]`.

lopopolo · 2023-05-22T00:42:33Z

clippy in pedantic mode reports an "unreadable literal" lint which suggests placing underscores in long literal numbers.

For example, this statement:

*cp = (b as u32 & 0b111111) | (*cp << 6);

is difficult to parse at first glance. It looked to me that the bit mask is a no-op, but with the underscore it's clear that the code masks off the top 2 bits of the byte:

*cp = (b as u32 & 0b0011_1111) | (*cp << 6);

lopopolo · 2023-05-22T00:48:44Z

These commits all fix pedantic lints and I'm a bit iffier on them. The transmute one I think is good though:

This is the CLI invocation to get a sense for what I've suppressed:

cargo clippy -- --warn clippy::pedantic \
    --allow clippy::module_name_repetitions \
    --allow clippy::must_use_candidate \
    --allow clippy::unnecessary_wraps \
    --allow clippy::missing_errors_doc \
    --allow clippy::match_same_arms \
    --allow clippy::enum_glob_use \
    --allow clippy::uninlined_format_args \
    --allow clippy::if_not_else \
    --allow clippy::single_match_else \
    --allow clippy::too_many_lines \
    --allow clippy::inline_always \
    --allow clippy::wildcard_imports \
    --allow clippy::similar_names \
    --allow clippy::ptr_as_ptr \
    --allow clippy::cast_ptr_alignment

Per upstream, the pedantic lints are intended to be liberally disabled if you disagree.

lopopolo · 2023-05-22T00:52:13Z

src/ascii.rs

 unsafe fn ptr_add(ptr: *const u8, amt: usize) -> *const u8 {
-    debug_assert!(amt < ::core::isize::MAX as usize);
-    ptr.offset(amt as isize)
+    ptr.add(amt)
 }

 /// Decrement the given pointer by the given amount.
 unsafe fn ptr_sub(ptr: *const u8, amt: usize) -> *const u8 {
-    debug_assert!(amt < ::core::isize::MAX as usize);
-    ptr.offset((amt as isize).wrapping_neg())
+    ptr.sub(amt)
 }


these two functions can probably be nuked and the routines from core slotted into place where they are used.

I believe these two functions on the pointer primitive were added during the great pointer reform of std.

lopopolo · 2023-05-22T00:53:42Z

src/impls.rs

@@ -479,7 +486,10 @@ mod bstr {
                    | '\x7f' => {
                        write!(f, "\\x{:02x}", ch as u32)?;
                    }
-                    '\n' | '\r' | '\t' | _ => {


with this change, I'd probably want to add a comment on why newline, carriage return, and tab are called out specially here to get their own match arm.

lopopolo · 2023-05-22T00:54:37Z

src/lib.rs

@@ -1,3 +1,5 @@
+#![allow(clippy::single_char_add_str)]


there are several places in the crate where the replacement character is used in a single character string. I prefer suppressing this because it means you don't pay the cost for a UTF-8 encode at runtime.

lopopolo · 2023-05-22T00:55:09Z

src/unicode/fsm/mod.rs

@@ -1,3 +1,5 @@
+#![allow(clippy::all)]


suppressed all lints because these modules appear to be generated code. clippy didn't like the use of the 'static lifetime in const definitions.

lopopolo · 2023-05-22T00:56:33Z

src/bstr.rs

-        unsafe { mem::transmute(slice) }
+        unsafe { &*(slice as *const [u8] as *const BStr) }
    }

    #[inline]
    pub(crate) fn from_bytes_mut(slice: &mut [u8]) -> &mut BStr {
-        unsafe { mem::transmute(slice) }
+        unsafe { &mut *(slice as *mut [u8] as *mut BStr) }


getting rid of the transmute seems like a win here. This cast soup could probably use a SAFETY comment that refers to the fact that BStr is repr(transparent) wrapper around [u8].

there is a clippy restriction lint that can be used to enforce that all unsafe blocks have safety comments: https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsafe_blocks

lopopolo · 2023-05-22T00:58:55Z

src/ext_slice.rs

-                let line = &self.bytes[..end + 1];
+                let line = &self.bytes[..=end];
                self.bytes = &self.bytes[end + 1..];


I would probably replace this suggestion and the code with a call to self.bytes.split_at(end). This would eliminate one bounds check and one panicking branch.

lopopolo · 2023-05-22T00:59:02Z

src/ext_slice.rs

                let line = &self.bytes[end + 1..];
-                self.bytes = &self.bytes[..end + 1];
+                self.bytes = &self.bytes[..=end];


lopopolo · 2023-05-22T00:59:38Z

src/ext_vec.rs

@@ -188,7 +188,7 @@ pub trait ByteVec: private::Sealed {
        fn imp(os_str: OsString) -> Result<Vec<u8>, OsString> {
            use std::os::unix::ffi::OsStringExt;


this code could be made to support wasi targets as well, which has a similar OsStringExt which is identical to the unix one.

lopopolo · 2023-05-22T01:00:16Z

src/ext_vec.rs

        #[cfg(unix)]
        #[inline]
-        fn imp<'a>(os_str: &'a OsStr) -> Cow<'a, [u8]> {
+        fn imp(os_str: &OsStr) -> Cow<'_, [u8]> {
            use std::os::unix::ffi::OsStrExt;


ditto, wasi support could be added.

lopopolo added 13 commits May 21, 2023 09:51

Address clippy::deprecated_cfg_attr lint violations

a3e76e6

Address clippy::get_first lint violations

8e629d5

Address clippy::needless_borrow lint violations

ab08a45

Address clippy::op_ref lint violations

584674d

Address clippy::needless_lifetimes lint violations

f46caa1

Address clippy::useless_conversion lint violations

9843ef7

Address clippy::map_clone lint violations

6a367f6

Simplify ByteSlice::last_byte

d9d7b88

Address clippy::redundant_static_lifetimes lint violations

5627aeb

Address clippy::while_let_on_iterator lint violations

948142f

Address clippy::needless_return lint violations

14db391

Address clippy::identity_op lint violations

9b7e161

Address clippy::ptr_offset_with_cast lint violations

906eb4b

lopopolo force-pushed the lopopolo/clippy branch from 8cecc7a to 74738ee Compare May 21, 2023 17:28

lopopolo commented May 21, 2023

View reviewed changes

lopopolo added 4 commits May 21, 2023 17:21

Address clippy::derived_hash_with_manual_eq lint violations

5b379ed

Allow clippy::single_char_add_str lint crate-wide

8bd170b

Address clippy::manual_find lint violations

76f590d

Address clippy::wildcard_in_or_patterns lint violations

246b823

lopopolo force-pushed the lopopolo/clippy branch from 74738ee to 246b823 Compare May 22, 2023 00:21

lopopolo added 5 commits May 21, 2023 17:25

use ptr.sub instead of ptr.offset with cast and assert

7b16270

Address clippy::transmute_ptr_to_ptr lint violations

39a749e

This is a pedantic lint, but it removes a transmute where a cast will do. The code is safe for the same reasons the transmute is safe: `BStr` is a repr transparent wrapper around `[u8]`.

Address clippy::range_plus_one lint violations

af7dba9

Address clippy::cloned_instead_of_copied lint violation

5c7c495

Address clippy::redundant_closure_for_method_calls lint violation

319b31a

Address clippy::cast_lossless and clippy::unreadable_literal lints

9800151

lopopolo commented May 22, 2023

View reviewed changes

lopopolo mentioned this pull request May 22, 2023

Use clippy in CI? #158

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address clippy lints #160

Address clippy lints #160

lopopolo commented May 21, 2023

lopopolo May 21, 2023

lopopolo commented May 22, 2023

lopopolo commented May 22, 2023 •

edited

lopopolo May 22, 2023

lopopolo May 22, 2023

lopopolo May 22, 2023

lopopolo May 22, 2023

lopopolo May 22, 2023

lopopolo May 22, 2023

lopopolo May 22, 2023 •

edited

lopopolo May 22, 2023

lopopolo May 22, 2023

lopopolo May 22, 2023

		@@ -188,7 +188,7 @@ pub trait ByteVec: private::Sealed {
		fn imp(os_str: OsString) -> Result<Vec<u8>, OsString> {
		use std::os::unix::ffi::OsStringExt;

Address clippy lints #160

Are you sure you want to change the base?

Address clippy lints #160

Conversation

lopopolo commented May 21, 2023

Choose a reason for hiding this comment

lopopolo commented May 22, 2023

lopopolo commented May 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lopopolo May 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lopopolo commented May 22, 2023 •

edited

lopopolo May 22, 2023 •

edited