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

Identity/erasing operation lints #2136

Merged
merged 4 commits into from
Oct 23, 2017

Conversation

ykrivopalov
Copy link

Yury Krivopalov added 2 commits October 14, 2017 12:21
For expressions that can be replaced by a zero.
@@ -58,15 +58,28 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IdentityOp {
}
}

fn no_zeros(v: &ConstInt) -> bool {
match *v {
ConstInt::I8(i) => i.count_zeros() == 0,
Copy link
Member

Choose a reason for hiding this comment

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

can we just compare with !0 or something?

Copy link
Author

Choose a reason for hiding this comment

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

Indeed, simplified

Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

Thank you! This is a good lint.

I only have a few nits, mostly about wording. I also came up with one possible extension, though I'll leave it to you if you want to implement it later or leave it to someone else.

/// **What it does:** Checks for erasing operations, e.g. `x * 0`.
///
/// **Why is this bad?** The whole expression can be replaced by zero.
/// Most likely mistake was made and code should be reviewed or simplified.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather write "This is most likely not the intended outcome and should probably be corrected."

declare_lint! {
pub ERASING_OP,
Warn,
"using erasing operations, e.g. `x * 0` or `y & 0`"
Copy link
Contributor

Choose a reason for hiding this comment

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

There is also x | -1 (signed) / x | !0 (unsigned), though those return -1 and !0 respectively. It's ok if we extend this later, though.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I can add this lint. We might also change the lint name. Something about expression with a constant result, I'm not sure.

cx,
ERASING_OP,
span,
"the operation is ineffective. Consider reducing it to `0`",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite like our message here, as it implies the zero would be correct, where this will most likely not be the case. I'd rather warn "this operation will always return zero. This is likely not the intended outcome."

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, fixed

@oli-obk
Copy link
Contributor

oli-obk commented Oct 23, 2017

Thanks!

@oli-obk oli-obk merged commit 2771378 into rust-lang:master Oct 23, 2017
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.

4 participants