Skip to content

New lint: iter_overeager_cloned #8202

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

Closed
pmnoxx opened this issue Dec 31, 2021 · 7 comments · Fixed by #8203
Closed

New lint: iter_overeager_cloned #8202

pmnoxx opened this issue Dec 31, 2021 · 7 comments · Fixed by #8203
Labels
A-lint Area: New lints

Comments

@pmnoxx
Copy link
Contributor

pmnoxx commented Dec 31, 2021

What it does

Calling .iter().cloned.last() involved doing unnecessary work.
It's better to write it as .iter().last().cloned()

Same thing can rule can be applied to:

  • .iter().cloned().next()
  • .iter().cloned().count()
  • .iter().cloned().flatten()
  • .iter().cloned().take(...)
  • .iter().cloned().skip(...)
  • .iter().cloned().nth(...)

Lint Name

cloned_last

Category

style, perf

Advantage

  • More efficient code

Drawbacks

  • I don't see any

Example

    let _: Option<String> = vec!["1".to_string(), "2".to_string(), "3".to_string()]
        .iter().cloned().last()

Could be written as:

    let _: Option<String> = vec!["1".to_string(), "2".to_string(), "3".to_string()]
        .iter().last().cloned()
@pmnoxx pmnoxx added the A-lint Area: New lints label Dec 31, 2021
@pmnoxx
Copy link
Contributor Author

pmnoxx commented Dec 31, 2021

I created a new PR for that lint: #8203

@pmnoxx pmnoxx changed the title New lint: cloned_last New lint: iter_overeager_cloned Jan 4, 2022
@bors bors closed this as completed in 93cad4a Jan 16, 2022
@kornelski
Copy link
Contributor

Iterators are lazy, so .iter().cloned().take(...) and .iter().cloned().next() are perfectly fine.

@llogiq
Copy link
Contributor

llogiq commented Jan 23, 2022

By themselves .cloned().take() is ok, but that could be followed by .last() and that would trigger the lint next time.

With .next() I somewhat agree, though I note that it will still produce more code to optimize away, so it will likely at least positively affect compile time (and probably debug runtime). Is there any benefit to not switching the order?

@kornelski
Copy link
Contributor

Ok, I see your point. The benefit would be only in number of warnings from clippy for a not-strictly-necessary case.

@robertbastian
Copy link
Contributor

I think the examples for this are bad and should be updated:

// Bad
vec.iter().cloned().take(10);

// Good
vec.iter().take(10).cloned();

These are equivalent, not a single clone happens. Even if we add .next(), only one clone will happen in both cases. collect would show it, or using skip:

// Bad
vec.iter().cloned().skip(10).next(); // 11 clones

// Good
vec.iter().skip(10).cloned().next(); // 0-1 clones

// Bad
vec.iter().cloned().last();

// Good
vec.iter().last().cloned();

This has a typo, the good line should be .clone().

@llogiq
Copy link
Contributor

llogiq commented Feb 18, 2022

No. Option::cloned() is the correct function to call here, and that's what the suggested code does.

@robertbastian
Copy link
Contributor

What about the first part of my comment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants