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

Lint for Min/Max If Statements #1006

Open
mmstick opened this issue Jun 12, 2016 · 5 comments
Open

Lint for Min/Max If Statements #1006

mmstick opened this issue Jun 12, 2016 · 5 comments
Labels
A-lint Area: New lints E-medium Call for participation: Medium difficulty level problem and requires some initial experience. L-style Lint: Belongs in the style lint group T-middle Type: Probably requires verifiying types

Comments

@mmstick
Copy link

mmstick commented Jun 12, 2016

Clippy should be able to detect instances where an if statement is being used to calculate the min or max of two or more values and recommend using the cmp::min/max functions instead. These two examples were taken from real-world code directly translated from C into Rust.

Example 1:

Unnecessary

let mut x = if a < b { a } else { b };
if x > y { x = y; }

Recommended

let x = cmp::min(cmp::min(a ,b), y);

Example 2:

Unnecessary

let mut count = (sum + stride / 2) / stride;
if count < 1 { count = 1; }
if sum == 0 { count = 0; }

Recommended

let count = if sum == 0 { 0 } else { cmp::max((sum + stride / 2) / stride, 1) };
@mcarton
Copy link
Member

mcarton commented Jun 12, 2016

let x = cmp::min(cmp::min(a ,b), y);

could even be [a, b, y].iter().min().unwrap() which is better IMO.

@mcarton mcarton added E-medium Call for participation: Medium difficulty level problem and requires some initial experience. T-AST Type: Requires working with the AST A-lint Area: New lints L-style Lint: Belongs in the style lint group labels Jun 12, 2016
@mmstick
Copy link
Author

mmstick commented Jun 12, 2016

Although I don't know how I feel about it requring unwrap(). In what scenario would it return None?

@mcarton
Copy link
Member

mcarton commented Jun 12, 2016

[].iter().min()

@mcarton mcarton added T-middle Type: Probably requires verifiying types and removed T-AST Type: Requires working with the AST labels Jun 12, 2016
@shepmaster
Copy link
Member

I'd disagree that the iterator version is better. It introduces the potential for failure (the unwrap) when the original one didn't have that potential. Chained min or max calls would be better.

@mcarton
Copy link
Member

mcarton commented Jun 13, 2016

The failure case would be using .iter().min() on an empty array, it's not a problem here. Anyway, we can also suggest both and let the user choose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints E-medium Call for participation: Medium difficulty level problem and requires some initial experience. L-style Lint: Belongs in the style lint group T-middle Type: Probably requires verifiying types
Projects
None yet
Development

No branches or pull requests

3 participants