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

Adds lint for integer division #4195

Merged
merged 1 commit into from
Jun 12, 2019

Conversation

thiagoarrais
Copy link
Contributor

Hi, folks! This is my first contribution to clippy and my first real piece of Rust code.

This is supposed to add a lint that warns for division of integers (#109). Please let me know if you need any changes.

Fixes #109

changelog: Add lint for integer division

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Just a NIT and this can get merged.

then {
let (left_ty, right_ty) = (cx.tables.expr_ty(left), cx.tables.expr_ty(right));
if left_ty.is_integral() && right_ty.is_integral() {
return true;
Copy link
Member

Choose a reason for hiding this comment

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

You can return this directly:

Suggested change
return true;
return left_ty.is_integral() && right_ty.is_integral();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done at b364eb7.

By the way, can we add a lint rule for that? Is this lintable?

Copy link
Member

@flip1995 flip1995 Jun 12, 2019

Choose a reason for hiding this comment

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

When I saw this, I also instantly that this could be linted. But this is pretty hard to lint and even harder / impossible inside a macro. This is only possible iff

if some_bool_expr {
    return true;
}

is the second to last expression and in every other case we return false. The needless_bool lint already lints the easy if _ { true } else { false } case. Maybe this could be extended. But this would have many edge cases.

@flip1995 flip1995 added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Jun 12, 2019
@thiagoarrais thiagoarrais force-pushed the division-of-integer-literals branch from 87ff215 to b364eb7 Compare June 12, 2019 12:38
@flip1995
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Jun 12, 2019

📌 Commit b364eb7 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Jun 12, 2019

⌛ Testing commit b364eb7 with merge a1eb60f...

bors added a commit that referenced this pull request Jun 12, 2019
…ip1995

Adds lint for integer division

Hi, folks! This is my first contribution to clippy and my first real piece of Rust code.

This is supposed to add a lint that warns for division of integers (#109). Please let me know if you need any changes.

Fixes #109

changelog: Add lint for integer division
@bors
Copy link
Contributor

bors commented Jun 12, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing a1eb60f to master...

@bors bors merged commit b364eb7 into rust-lang:master Jun 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint for warning on division of integer literals
3 participants