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

Adding 'clone to satisfy the borrow checker' anti-pattern #23

Closed
wants to merge 2 commits into from

Conversation

liamzdenek
Copy link

I feel like this write-up could be improved, but I'm not exactly sure how -- recommendations and modifications are welcome.


## Example

```rust fn main() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The "rust" and "fn main()" should be on separate lines, like this:

```rust
fn main() {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, I don't think you need to wrap the code in fn main() -- the other examples in this repo (along with the official docs) keep the code bare.

// x has been borrowed
// thanks to x.clone(), x was never borrowed, and this line will run.
println!("{}", x);
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can come up with a better example; I'm not sure if this is really a problem when your type is copy. Let me play around with some examples and get back to you.

@cbreeden
Copy link
Collaborator

cbreeden commented Sep 21, 2016

I like your motivation and description. I think we can make improve this section quite a bit if we put a little more polish into the example, and add examples that shows how you can avoid the problems. I'll play around with a few ideas and get back to you.

@cbreeden
Copy link
Collaborator

cbreeden commented Sep 21, 2016

So I tried to think of some good examples, what do you think about this one

// Our personal key-value storage for names and ages
let mut vec = vec![("Bob".to_string(), 17), ("Stacy".to_string(), 21)];
let new_pair = ("Ferris".to_string(), 13);

// check for duplicates
let (ref key, _) = new_pair;
if vec.iter().map(|&(ref k, _)| k).find(|&k| k == key).is_none() {
    // Can't move since new_pair is being referenced still!
    vec.push(new_pair);
}

println!("{:?}", vec);

I could imagine someone trying to add a .clone() to the new_pair in vec.push(new_pair);. A lot of times these problems can be solved by properly managing their lifetimes -- in this case just adding k == &new_pair.0. In other cases, closing a mutable borrow scope in brackets. I don't know if this is the best example, just throwing it out there.

@cbreeden cbreeden mentioned this pull request Oct 8, 2016
@llogiq
Copy link
Contributor

llogiq commented Nov 12, 2016

I have a few ideas for examples:

  • change an enum value, leaving parts intact (see new idiom: mem::replace #31)
  • select & remove items from a collection (use Drain instead once available)
  • un-by-ref an iterator (.cloned()) which is actually OK in many cases (unless you can consume the collection while iterating, which should be preferred)
  • entry(_.clone()) – there's an RFC to get rid of that.

Hope that helps.

@pickfire
Copy link
Contributor

Anything left for this?

@simonsan simonsan added the C-addition Category: Adding new content, something that didn't exist in the repository before label Dec 31, 2020
@simonsan
Copy link
Collaborator

@liamzdenek any updates on this?

@simonsan simonsan added the C-waiting for Category: Waiting for feedback of the initial author or some external dependency/issue label Dec 31, 2020
@simonsan
Copy link
Collaborator

simonsan commented Jan 1, 2021

Closing due to inactivity. Further additions to the PR can be made in #110.

@simonsan simonsan closed this Jan 1, 2021
@simonsan simonsan mentioned this pull request Jan 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-addition Category: Adding new content, something that didn't exist in the repository before C-waiting for Category: Waiting for feedback of the initial author or some external dependency/issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants