-
Notifications
You must be signed in to change notification settings - Fork 892
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Run clippy on CI #2315
Run clippy on CI #2315
Conversation
ae6aee5
to
938ef8d
Compare
c2718c4
to
d0bce5a
Compare
src/platform_impl/ios/window.rs
Outdated
@@ -452,7 +452,7 @@ impl Window { | |||
// Like the Windows and macOS backends, we send a `ScaleFactorChanged` and `Resized` | |||
// event on window creation if the DPI factor != 1.0 | |||
let scale_factor: CGFloat = msg_send![view, contentScaleFactor]; | |||
let scale_factor: f64 = scale_factor.into(); | |||
let scale_factor: f64 = scale_factor as _; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable type was removed in a similar expression higher up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what do you want from me here. The problem is that CGFLoat is either f32 or f64, but winit API is f64, so we need to cast it to f64 here. Using .into()
won't work, since clippy finds it as redundant when CGFloat
is f64
, however we can cast it like as _
and explicitly state that we assign to f64
value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think because this change was made in view.rs
:
- let scale_factor: f64 = scale_factor.into();
+ let scale_factor = scale_factor as f64;
Isn't that affected in the same way?
FrameExtentsHeuristic { | ||
frame_extents, | ||
heuristic_path: UnsupportedNested, | ||
} | ||
} else { | ||
// This is the case for xmonad and dwm, AKA the only WMs tested that supplied a | ||
// border value. This is convenient, since we can use it to get an accurate frame. | ||
let frame_extents = FrameExtents::from_border(border.into()); | ||
let frame_extents = FrameExtents::from_border(border as c_ulong); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which clippy gets triggered here ?
PS: the best is to do one commit per clippy type...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we call into()
to convert from u64
to u64
. The problem is that the code was correct, it just bitness dependent, so it's a clippy warning on 64-bit, but works fine on 32-bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS: the best is to do one commit per clippy type...
That's very hard and wasteful if you're tackling them all at once using clippy --fix
, unless collecting all the individual warnings and running clippy --fix
one by one, allowing all the lints except one.
@MarijnS95 Do you happen to know why android target randomly fails from time to time? I think I've seen it a bunch of times here, rerunning do fix the issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iOS and macOS changes looks fine (I never did like the let () = msg_send![...]
, so cool that Clippy doesn't either).
@@ -132,6 +132,9 @@ | |||
|
|||
#![deny(rust_2018_idioms)] | |||
#![deny(rustdoc::broken_intra_doc_links)] | |||
#![deny(clippy::all)] | |||
#![cfg_attr(feature = "cargo-clippy", deny(warnings))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clippy
enables the cargo-clippy
feature on all crates when running through it, automatically denying all rustc
warnings in this instance. However, this implies warnings from clippy::all
(that lint group should only include warn/deny lints) and the #![deny(clippy::all)]
is thus superfluous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deny clippy all means that it'll error on clippy warnings instead of treating them as warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that's also what #![cfg_attr(feature = "cargo-clippy", deny(warnings))]
does :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think those about rustc warnings, and not clippy onces? I think it was like that in the past, at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it and it includes all warnings, including clippy ones.
#[derive(Clone, Debug, PartialEq)] | ||
#[derive(Clone, Debug, PartialEq, Eq)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Add changelog entry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think internal impl requires it? Also I'm not sure that we should add changelog notes for Eq
impls, since they are not breaking the code and transparent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internal impls don't matter.
It's not a breaking change, but in general I think all API changes should be documented in the changelog, including small ones like this. But I don't have a strong opinion on this, you're free to disagree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A changelog entry would indeed be nice.
.github/workflows/ci.yml
Outdated
- name: Lint with clippy | ||
shell: bash | ||
if: (matrix.rust_version != 'nightly') && !contains(matrix.platform.options, '--no-default-features') | ||
run: cargo clippy --all-targets --target ${{ matrix.platform.target }} $OPTIONS --features $FEATURES |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As said in the glutin
PR, I'd like to keep -- -Dwarnings
in here so that new crates and bin (example/test/bench) targets are included too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't want to run clippy for examples actually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be consistent and enforce it everywhere, so that the examples are also clean-enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition there's --all-targets
here and you fixed+allowed clippy lints in the examples/
, so this is only sensible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add -D warnings
on CI then, I guess.
Having deny in lib.rs is still easier to understand that you must fix those issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, leave the deny in lib.rs
, that should hopefully decrease the amount of noise in PRs that would otherwise require lots of cleanup pushes to "make the CI happy".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fairly certain that the general advice here is to not deny warnings in *.rs
files, and instead only do it in CI, as it might break compilation with future versions of rustc
and/or clippy
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the point, we don't deny warnings for rustc or any related command, just when running clippy. So there's no way the build will break, only clippy could, but it would only mean that CI is broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. That's clever.
src/platform_impl/ios/window.rs
Outdated
@@ -452,7 +452,7 @@ impl Window { | |||
// Like the Windows and macOS backends, we send a `ScaleFactorChanged` and `Resized` | |||
// event on window creation if the DPI factor != 1.0 | |||
let scale_factor: CGFloat = msg_send![view, contentScaleFactor]; | |||
let scale_factor: f64 = scale_factor.into(); | |||
let scale_factor: f64 = scale_factor as _; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think because this change was made in view.rs
:
- let scale_factor: f64 = scale_factor.into();
+ let scale_factor = scale_factor as f64;
Isn't that affected in the same way?
FrameExtentsHeuristic { | ||
frame_extents, | ||
heuristic_path: UnsupportedNested, | ||
} | ||
} else { | ||
// This is the case for xmonad and dwm, AKA the only WMs tested that supplied a | ||
// border value. This is convenient, since we can use it to get an accurate frame. | ||
let frame_extents = FrameExtents::from_border(border.into()); | ||
let frame_extents = FrameExtents::from_border(border as c_ulong); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS: the best is to do one commit per clippy type...
That's very hard and wasteful if you're tackling them all at once using clippy --fix
, unless collecting all the individual warnings and running clippy --fix
one by one, allowing all the lints except one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like all my comments were addressed minus some minor nits which we can continue to discuss, nice job!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any further blocking concerns, although it would be nice to have changelog entries for all the added trait impls on public types.
Android failures were unrelated and quite random recently. |
@@ -1,4 +1,5 @@ | |||
#![cfg(target_os = "macos")] | |||
#![allow(clippy::let_unit_value)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That we have to specify this is actually a regression, I've filed an issue about it here: rust-lang/rust-clippy#8998.
Fixes #1402.