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

Add lint to detect manual slice copies #2021

Merged
merged 3 commits into from
Sep 5, 2017

Conversation

marcusklaas
Copy link
Contributor

This PR replaces #1937.

Addresses issue #1831.

I see there are still some formatting issues. This PR has been formatted with the latest rustfmt (stable). I am receptive to instructions on how to fix these.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 5, 2017

This PR has been formatted with the latest rustfmt (stable).

It looks like you actually ran both (first nightly, then stable).

I am receptive to instructions on how to fix these.

So I applied the method that I'd desire in situations like these directly to the PR:

  1. A commit doing all the formatting without touching the files in any non-formatting way
  2. A commit adding tests showing the false positives
  3. A commit with the fixes and the updated tests

This makes it really easy to see the changes you did in the last commit, since the ui-tests don't show any line change noise

@oli-obk
Copy link
Contributor

oli-obk commented Sep 5, 2017

Did you ever address

After a more detailed inspection of the documentation on clone_from_slice, I think the current lint suggestions may cause panics. The destination slice must apparently be exactly as long as the source. Also, it may be preferable to use copy_from_slice when T: Copy, although in my experience clone_from_slice also compiles to a memcpy in release builds.

in the previous PR? This PR looks good to me with the suggestion fixed to produce the same semantics as the loop.

@marcusklaas
Copy link
Contributor Author

marcusklaas commented Sep 5, 2017

Thanks for the commits (I didn't know that was possible!) and the guidance!

I did address the panic situation. The destination slice should now always be as long as the source slice, although I did not make a separate commit for it.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 5, 2017

Great!

Another lint that will have a positive impact on many projects. So much cool stuff in clippy lately

@oli-obk oli-obk merged commit 2e33285 into rust-lang:master Sep 5, 2017
@marcusklaas
Copy link
Contributor Author

👍

@marcusklaas marcusklaas deleted the needless-loop-2 branch September 5, 2017 16:42
@Manishearth
Copy link
Member

Wow this is a pretty sweet lint

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.

3 participants