fix more clippy warnings #7251
fix more clippy warnings #7251
Conversation
|
r? @ehuss (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
matthiaskrgr
force-pushed
the
clippy_v6
branch
from
88863ce to
596ed63
Compare
|
@bors: r+ |
|
📌 Commit 596ed63 has been approved by |
bors
added
S-waiting-on-bors
src/cargo/core/shell.rs
Outdated
| unsafe { | ||
| let mut winsize: libc::winsize = mem::zeroed(); | ||
| if libc::ioctl(libc::STDERR_FILENO, libc::TIOCGWINSZ.into(), &mut winsize) < 0 { | ||
| if libc::ioctl(libc::STDERR_FILENO, libc::TIOCGWINSZ, &mut winsize) < 0 { |
There was a problem hiding this comment.
This was changed specifically for freebsd, I don't think it should be modified.
There was a problem hiding this comment.
Alright, I backed that one out.
There was a problem hiding this comment.
Can you add #[allow(clippy::identity_conversion)] with a comment explaining why?
There was a problem hiding this comment.
FWIW I'm personally not a fan of adding #[allow(clippy::...)] throughout the code, I find it pretty jarring to basically see a random distribution of clippy lints sprayed throughout the code. I think it's pretty bad style and is one of the reasons I'm not a fan of clippy, I'd rather not have a tool force me to randomly annotate code where it just otherwise causes bugs.
There was a problem hiding this comment.
Would it be OK to add it to src/cargo/lib.rs with the rest of the list?
I agree there are too many false positives in clippy's default set, and that it can be too pedantic. However, I think there are enough cases where it does help simplify and clarify code, and even point out invalid code. For example, it points out a memory corruption issue in git2 that I've been meaning to fix, but haven't got around to.
I also think there should have been a comment here in the first place. A magical .into that only applies to one target that isn't even tested on CI should have something to explain why it is there.
There was a problem hiding this comment.
Yes I think it'd at least be better to go in src/cargo/lib.rs. I also do understand that this is a pretty unproductive location to litigate this, it's not like anyone from clippy is watching this PR. I apologize for venting here :(
There was a problem hiding this comment.
I suppressed the warning in lib.rs and reverted the change to shell.rs.
|
@bors: r- |
bors
added
S-waiting-on-author
|
☀️ Try build successful - checks-azure |
matthiaskrgr
force-pushed
the
clippy_v6
branch
3 times, most recently
from
d20c78e to
ccc7684
Compare
matthiaskrgr
force-pushed
the
clippy_v6
branch
from
ccc7684 to
61c1b1c
Compare
|
Thanks, I pushed a small comment addition. @bors r+ |
|
📌 Commit e3bb695 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-azure |
5 participants
No description provided.