feat: simplify manual bits extraction and an unneeded reref#484
Merged
Byron merged 1 commit intorust-lang:mainfrom Apr 27, 2025
Merged
Conversation
Byron
approved these changes
Apr 27, 2025
Member
Byron
left a comment
There was a problem hiding this comment.
Thanks a lot, much appreciated!
| *pos += read_into(self.reader.get_mut().get_mut(), &mut buf[*pos..])?; | ||
| } else { | ||
| let (crc, amt) = finish(&buf); | ||
| let (crc, amt) = finish(buf); |
Member
There was a problem hiding this comment.
It's strange that clippy didn't catch this one before.
There are, however, many other warnings might be something you are interested in fixing/improving as well.
clippy log
❯ cargo clippy
Updating crates.io index
Locking 13 packages to latest compatible versions
Adding bitflags v2.9.0
Updating cloudflare-zlib-sys v0.3.3 -> v0.3.6
Adding getrandom v0.3.2
Updating libc v0.2.153 -> v0.2.172
Updating libz-rs-sys v0.4.1 -> v0.5.0
Updating miniz_oxide v0.8.0 -> v0.8.8
Adding r-efi v5.2.0
Adding rand v0.9.1
Updating rand_chacha v0.3.1 -> v0.9.0
Adding rand_core v0.9.3
Adding wasi v0.14.2+wasi-0.2.4
Adding wit-bindgen-rt v0.39.0
Updating zlib-rs v0.4.1 -> v0.5.0
Downloaded crc32fast v1.4.0
Downloaded 1 crate (38.7 KB) in 0.19s
Compiling crc32fast v1.4.0
Checking adler2 v2.0.0
Checking cfg-if v1.0.0
Checking miniz_oxide v0.8.8
Checking flate2 v1.1.1 (/Users/byron/dev/github.com/rust-lang/flate2-rs)
warning: this operation has no effect
--> src/gz/bufread.rs:89:13
|
89 | (crc.amount() >> 0) as u8,
| ^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `crc.amount()`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#identity_op
= note: `#[warn(clippy::identity_op)]` on by default
warning: this operation has no effect
--> src/gz/bufread.rs:120:15
|
120 | let crc = ((buf[0] as u32) << 0)
| ^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `((buf[0] as u32))`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#identity_op
warning: this operation has no effect
--> src/gz/bufread.rs:124:15
|
124 | let amt = ((buf[4] as u32) << 0)
| ^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `((buf[4] as u32))`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#identity_op
warning: this expression creates a reference which is immediately dereferenced by the compiler
--> src/gz/bufread.rs:306:49
|
306 | let (crc, amt) = finish(&buf);
| ^^^^ help: change this to: `buf`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
= note: `#[warn(clippy::needless_borrow)]` on by default
warning: this operation has no effect
--> src/gz/write.rs:96:17
|
96 | (sum >> 0) as u8,
| ^^^^^^^^^^ help: consider reducing it to: `sum`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#identity_op
warning: this operation has no effect
--> src/gz/write.rs:100:17
|
100 | (amt >> 0) as u8,
| ^^^^^^^^^^ help: consider reducing it to: `amt`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#identity_op
warning: this operation has no effect
--> src/gz/write.rs:297:19
|
297 | let crc = ((self.crc_bytes[0] as u32) << 0)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `((self.crc_bytes[0] as u32))`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#identity_op
warning: this operation has no effect
--> src/gz/write.rs:301:19
|
301 | let amt = ((self.crc_bytes[4] as u32) << 0)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `((self.crc_bytes[4] as u32))`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#identity_op
warning: this operation has no effect
--> src/gz/mod.rs:138:41
|
138 | self.header.mtime = ((buffer[4] as u32) << 0)
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `((buffer[4] as u32))`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#identity_op
warning: this operation has no effect
--> src/gz/mod.rs:405:25
|
405 | header.push((v.len() >> 0) as u8);
| ^^^^^^^^^^^^^^ help: consider reducing it to: `v.len()`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#identity_op
warning: this operation has no effect
--> src/gz/mod.rs:421:21
|
421 | header[4] = (mtime >> 0) as u8;
| ^^^^^^^^^^^^ help: consider reducing it to: `mtime`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#identity_op
warning: casting to the same type is unnecessary (`isize` -> `isize`)
--> src/mem.rs:50:12
|
50 | None = ffi::MZ_NO_FLUSH as isize,
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `ffi::MZ_NO_FLUSH`
|
= 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 (`isize` -> `isize`)
--> src/mem.rs:60:15
|
60 | Partial = ffi::MZ_PARTIAL_FLUSH as isize,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `ffi::MZ_PARTIAL_FLUSH`
|
= 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 (`isize` -> `isize`)
--> src/mem.rs:69:12
|
69 | Sync = ffi::MZ_SYNC_FLUSH as isize,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `ffi::MZ_SYNC_FLUSH`
|
= 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 (`isize` -> `isize`)
--> src/mem.rs:76:12
|
76 | Full = ffi::MZ_FULL_FLUSH as isize,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `ffi::MZ_FULL_FLUSH`
|
= 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 (`isize` -> `isize`)
--> src/mem.rs:82:14
|
82 | Finish = ffi::MZ_FINISH as isize,
| ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `ffi::MZ_FINISH`
|
= 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 (`isize` -> `isize`)
--> src/mem.rs:93:12
|
93 | None = ffi::MZ_NO_FLUSH as isize,
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `ffi::MZ_NO_FLUSH`
|
= 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 (`isize` -> `isize`)
--> src/mem.rs:102:12
|
102 | Sync = ffi::MZ_SYNC_FLUSH as isize,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `ffi::MZ_SYNC_FLUSH`
|
= 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 (`isize` -> `isize`)
--> src/mem.rs:108:14
|
108 | Finish = ffi::MZ_FINISH as isize,
| ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `ffi::MZ_FINISH`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
warning: `flate2` (lib) generated 19 warnings (run `cargo clippy --fix --lib -p flate2` to apply 19 suggestions)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 2.30s
Contributor
Author
There was a problem hiding this comment.
Yeah, I'm working my way up to solve all of them (:
| flg |= FEXTRA; | ||
| header.push((v.len() >> 0) as u8); | ||
| header.push((v.len() >> 8) as u8); | ||
| header.extend((v.len() as u16).to_le_bytes()); |
Member
There was a problem hiding this comment.
I think I remember that in another PR with a similar change you could even validate that the assembly remains the same, so I'd think the same is happening here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
finishalready takes in a reference, so this creates a needless borrow which is dereferenced at compile time.is exactly the same as doing
because we dont need the full length anyways, I find the latter simpler, LLVM should solve these as exactly the same operations with optimizations on.