Skip to content
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

Fix a few new clippy warnings #2090

Merged
merged 1 commit into from Oct 11, 2022
Merged

Fix a few new clippy warnings #2090

merged 1 commit into from Oct 11, 2022

Conversation

kaivol
Copy link
Contributor

@kaivol kaivol commented Oct 9, 2022

removed some unnecessary casts (clippy::unnecessary-cast) and ignored another warning (clippy::manual_clamp)

@ChrisDenton
Copy link
Collaborator

Is there a reason not to use clamp here?

@riverar
Copy link
Collaborator

riverar commented Oct 10, 2022

Sounds reasonable. Can we also change the title of the PR to reflect what was changed?

@kennykerr
Copy link
Collaborator

kennykerr commented Oct 10, 2022

Is there a reason not to use clamp here?

The trouble is that clamp will panic if min > max. You can test it by resizing the window so that the radius gets too small to fit the client area.

@kennykerr kennykerr changed the title Make clippy happier Fix a few new clippy warnings Oct 10, 2022
@@ -179,6 +179,7 @@ impl Window {

let size = unsafe { target.GetSize() };

#[allow(clippy::manual_clamp)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a clippy bug that this warning is not appropriate and must be suppressed.

@kennykerr kennykerr merged commit 67564c7 into microsoft:master Oct 11, 2022
@kaivol kaivol deleted the clippy branch October 11, 2022 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants