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

Confusing cmp_owned false positive with <u32 as From<midly::primitive::u28>>::from #8803

Closed
PatchMixolydic opened this issue May 9, 2022 · 1 comment · Fixed by #8807
Closed
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@PatchMixolydic
Copy link
Contributor

PatchMixolydic commented May 9, 2022

Summary

Unfortunately, I couldn't minimize it, but I tried linting this code. Specifically, this is the area that gave me grief:

// event.delta: midly::primitive::u28
// ticks_since_last_update: &mut u32; comes from an iterator
if u32::from(event.delta) > *ticks_since_last_update {
    // Not ready to proceed yet
    break;
}

I saw this happen:

warning: this creates an owned instance just for comparison
  --> src/lib.rs:96:24
   |
96 |                     if u32::from(event.delta) > *ticks_since_last_update {
   |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ try implementing the comparison without allocating
   |
   = note: `#[warn(clippy::cmp_owned)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#cmp_owned

I expected to see this happen:

Nothing. This comparison is fine as-is since u32: Copy, and removing the deref fails to compile. What might be the issue? Strangely enough, removing u32::from silences the lint, even though From<midly::primitive::u28> for u32 doesn't allocate (it just unwraps a newtype, which only copies a u32).

Lint Name

cmp_owned

Reproducer

See above

Version

rustc 1.62.0-nightly (1f7fb6413 2022-04-10)
binary: rustc
commit-hash: 1f7fb6413d6d6c0c929b223e478e44c3db991b03
commit-date: 2022-04-10
host: x86_64-unknown-linux-gnu
release: 1.62.0-nightly
LLVM version: 14.0.0

Additional Labels

No response

@PatchMixolydic PatchMixolydic added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels May 9, 2022
@PatchMixolydic PatchMixolydic changed the title Confusing cmp_owned false positive Confusing cmp_owned false positive with <u32 as From<midly::primitive::u28>>::from May 9, 2022
@Jarcho
Copy link
Contributor

Jarcho commented May 9, 2022

cmp_owned triggers when From::from is used in a comparison which could be done without it. Over-zealous in this case as converting to a u32 shouldn't ever need an allocation.

I would say the comparison is better done without the conversion anyways, but that's a style thing rather than a performance issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants