Enforce even more library clippy lints in CI - #161328
Conversation
|
Btw, they'll likely be platform specific fixes that CI tells me about so I'll probably be updating these in a few hours. |
| match cond { | ||
| crate::option::Option::Some(cond) => { | ||
| if !cond(&ret) { | ||
| // Emit no unwind panic in case this was a safety requirement. | ||
| crate::panicking::panic_nounwind("failed ensures check"); | ||
| } | ||
| }, | ||
| crate::option::Option::None => {}, | ||
| if let crate::option::Option::Some(cond) = cond { | ||
| if !cond(&ret) { | ||
| // Emit no unwind panic in case this was a safety requirement. | ||
| crate::panicking::panic_nounwind("failed ensures check"); | ||
| } |
There was a problem hiding this comment.
This is the intrinsic that got people pinged (sorry). If you rather I didn't change this then please say!
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Enforce even more library clippy lints in CI
| if next_read != next_write { | ||
| let ptr_write = prev_ptr_write.add(1); | ||
| mem::swap(&mut *ptr_read, &mut *ptr_write); | ||
| ptr::swap(ptr_read, ptr_write); |
There was a problem hiding this comment.
This should probably be looked at closer. Clippy says these do the same thing but it's possible going via an &mut is doing something useful.
There was a problem hiding this comment.
mem::swap will emit noalias so it assumes the areas don't overlap (thus better opts). I would say it's fine to use swap_nonoverlapping to make clippy happy as well here fwiw
There was a problem hiding this comment.
Hm, it might be better to not enforce this lint since it's so situational.
There was a problem hiding this comment.
this may be a deficiency in the clippy lint? &mut implies nonoverlapping, the lint should probably account for that.
There was a problem hiding this comment.
mostly looks good, with a couple worries:
- clippy wants us to do a lot more method calls. this is fine but needs a perf run for sure, and i'm a smidge worried about binary size and/or bootstrap time, esp since it might put more pressure on the inliner
- a lot of these
allows should beexpects with a comment... but also, i'm not gonna do that to you chris ^^ that can be a later pr
let's see what a perf run comes up with before anything.
@bors try @rust-timer queue
| @@ -1,5 +1,7 @@ | |||
| //! impl char {} | |||
|
|
|||
| #![allow(clippy::manual_is_ascii_check)] | |||
There was a problem hiding this comment.
Is this lint worth adding, given that it seems the only change is, uh, allowing it anyway? :D
I suppose there's the point of "let's add this so it doesn't show up in the future" which is fair but I'd like to know if that is indeed your specific intent here
There was a problem hiding this comment.
Yeah. A lot of these #[allow] are to stop clippy complaining about the implementation of things it's linting for so when you run x clippy library you don't get a bunch of spurious warnings. I guess it is possible clippy might one day get smarter about this but it's more an us problem rather than a clippy problem per se.
| ret | ||
| } else { | ||
| match cond { | ||
| crate::option::Option::Some(cond) => { |
There was a problem hiding this comment.
why isn't this if let $whatever && !cond? of all things I'd expect clippy to yell about this nested if :D
There was a problem hiding this comment.
Clippy moves in mysterious ways. Though it is entirely possible I missed the particular lint that would complain about it.
This comment has been minimized.
This comment has been minimized.
Enforce even more library clippy lints in CI
|
huh, how did the comment from matthias not show up? apologies ^^ github has been odd |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Just comparing to my personal clippy lint list, what I see missing is:
Besides |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (8906bf7): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 0.9%, secondary 0.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis perf run didn't have relevant results for this metric. Binary sizeResults (primary -0.1%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 456.919s -> 457.665s (0.16%) |
I went ahead and added a bunch of expect messages. |
View all comments
Once again, this is best reviewed commit-by-commit since each commit fixes a lint.
I've bundled together a large(ish) number of lints because I feel they shouldn't be controversial and don't touch too much code. A large chunk of them is just telling clippy that, for example, implementing
is_digitby callingis_digitjust isn't going to work. I have reviewed each and every fix myself but please do double or triple check my working!